Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add strict_override_decorator option (PEP 698) #15512

Merged
merged 7 commits into from
Jul 13, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/source/class_basics.rst
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,12 @@ show an error:
def g(self, y: str) -> None: # Error: no corresponding base method found
...

.. note::

Use ``--strict-override-decorator`` or
:confval:`strict_override_decorator = True <strict_override_decorator>` to require
methods overrides use the ``@override`` decorator. Emit an error if it is missing.
cdce8p marked this conversation as resolved.
Show resolved Hide resolved

You can also override a statically typed method with a dynamically
typed one. This allows dynamically typed code to override methods
defined in library classes without worrying about their type
Expand Down
7 changes: 7 additions & 0 deletions docs/source/config_file.rst
Original file line number Diff line number Diff line change
Expand Up @@ -714,6 +714,13 @@ section of the command line docs.
Prohibit equality checks, identity checks, and container checks between
non-overlapping types.

.. confval:: strict_override_decorator
cdce8p marked this conversation as resolved.
Show resolved Hide resolved

:type: boolean
:default: False

Require ``override`` decorator if method is overriding a base class method.
cdce8p marked this conversation as resolved.
Show resolved Hide resolved

.. confval:: strict

:type: boolean
Expand Down
47 changes: 37 additions & 10 deletions mypy/checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -643,9 +643,17 @@ def _visit_overloaded_func_def(self, defn: OverloadedFuncDef) -> None:
if defn.impl:
defn.impl.accept(self)
if defn.info:
found_base_method = self.check_method_override(defn)
if defn.is_explicit_override and found_base_method is False:
found_method_base_classes = self.check_method_override(defn)
if defn.is_explicit_override and not found_method_base_classes:
cdce8p marked this conversation as resolved.
Show resolved Hide resolved
self.msg.no_overridable_method(defn.name, defn)
elif (
found_method_base_classes
and self.options.strict_override_decorator
and not defn.is_explicit_override
cdce8p marked this conversation as resolved.
Show resolved Hide resolved
):
self.msg.override_decorator_missing(
defn.name, found_method_base_classes[0].fullname, defn.impl or defn
)
self.check_inplace_operator_method(defn)
if not defn.is_property:
self.check_overlapping_overloads(defn)
Expand Down Expand Up @@ -972,7 +980,15 @@ def _visit_func_def(self, defn: FuncDef) -> None:
# overload, the legality of the override has already
# been typechecked, and decorated methods will be
# checked when the decorator is.
self.check_method_override(defn)
found_method_base_classes = self.check_method_override(defn)
if (
found_method_base_classes
and self.options.strict_override_decorator
and defn.name not in ("__init__", "__new__")
):
self.msg.override_decorator_missing(
defn.name, found_method_base_classes[0].fullname, defn
)
self.check_inplace_operator_method(defn)
if defn.original_def:
# Override previous definition.
Expand Down Expand Up @@ -1814,23 +1830,26 @@ def expand_typevars(
else:
return [(defn, typ)]

def check_method_override(self, defn: FuncDef | OverloadedFuncDef | Decorator) -> bool | None:
def check_method_override(
self, defn: FuncDef | OverloadedFuncDef | Decorator
) -> list[TypeInfo] | None:
"""Check if function definition is compatible with base classes.

This may defer the method if a signature is not available in at least one base class.
Return ``None`` if that happens.

Return ``True`` if an attribute with the method name was found in the base class.
Return a list of base classes which contain an attribute with the method name.
"""
# Check against definitions in base classes.
found_base_method = False
found_method_base_classes: list[TypeInfo] = []
for base in defn.info.mro[1:]:
result = self.check_method_or_accessor_override_for_base(defn, base)
if result is None:
# Node was deferred, we will have another attempt later.
return None
found_base_method |= result
return found_base_method
if result:
found_method_base_classes.append(base)
return found_method_base_classes

def check_method_or_accessor_override_for_base(
self, defn: FuncDef | OverloadedFuncDef | Decorator, base: TypeInfo
Expand Down Expand Up @@ -4740,9 +4759,17 @@ def visit_decorator(self, e: Decorator) -> None:
self.check_incompatible_property_override(e)
# For overloaded functions we already checked override for overload as a whole.
if e.func.info and not e.func.is_dynamic() and not e.is_overload:
found_base_method = self.check_method_override(e)
if e.func.is_explicit_override and found_base_method is False:
found_method_base_classes = self.check_method_override(e)
if e.func.is_explicit_override and not found_method_base_classes:
self.msg.no_overridable_method(e.func.name, e.func)
elif (
found_method_base_classes
and self.options.strict_override_decorator
and not e.func.is_explicit_override
cdce8p marked this conversation as resolved.
Show resolved Hide resolved
):
self.msg.override_decorator_missing(
e.func.name, found_method_base_classes[0].fullname, e.func
)

if e.func.info and e.func.name in ("__init__", "__new__"):
if e.type and not isinstance(get_proper_type(e.type), (FunctionLike, AnyType)):
Expand Down
8 changes: 8 additions & 0 deletions mypy/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -824,6 +824,14 @@ def add_invertible_flag(
group=strictness_group,
)

add_invertible_flag(
"--strict-override-decorator",
default=False,
strict_flag=False,
help="Require override decorator if method is overriding a base class method.",
cdce8p marked this conversation as resolved.
Show resolved Hide resolved
group=strictness_group,
)

add_invertible_flag(
"--extra-checks",
default=False,
Expand Down
7 changes: 7 additions & 0 deletions mypy/messages.py
Original file line number Diff line number Diff line change
Expand Up @@ -1525,6 +1525,13 @@ def no_overridable_method(self, name: str, context: Context) -> None:
context,
)

def override_decorator_missing(self, name: str, base_name: str, context: Context) -> None:
self.fail(
f'Method "{name}" is not marked as override '
cdce8p marked this conversation as resolved.
Show resolved Hide resolved
f'but is overriding a method in class "{base_name}"',
context,
)

def final_cant_override_writable(self, name: str, ctx: Context) -> None:
self.fail(f'Cannot override writable attribute "{name}" with a final one', ctx)

Expand Down
4 changes: 4 additions & 0 deletions mypy/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ class BuildType:
"strict_concatenate",
"strict_equality",
"strict_optional",
"strict_override_decorator",
"warn_no_return",
"warn_return_any",
"warn_unreachable",
Expand Down Expand Up @@ -200,6 +201,9 @@ def __init__(self) -> None:
# This makes 1 == '1', 1 in ['1'], and 1 is '1' errors.
self.strict_equality = False

# Require override decorator. Strict mode for PEP 698.
self.strict_override_decorator = False

# Deprecated, use extra_checks instead.
self.strict_concatenate = False

Expand Down
123 changes: 104 additions & 19 deletions test-data/unit/check-functions.test
Original file line number Diff line number Diff line change
Expand Up @@ -2752,8 +2752,7 @@ class E(D): pass
class F(E):
@override
def f(self, x: int) -> str: pass
[typing fixtures/typing-full.pyi]
[builtins fixtures/tuple.pyi]
[typing fixtures/typing-override.pyi]

[case explicitOverrideStaticmethod]
# flags: --python-version 3.12
Expand Down Expand Up @@ -2785,8 +2784,8 @@ class D(A):
def f(x: str) -> str: pass # E: Argument 1 of "f" is incompatible with supertype "A"; supertype defines the argument type as "int" \
# N: This violates the Liskov substitution principle \
# N: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
[typing fixtures/typing-full.pyi]
[builtins fixtures/callable.pyi]
[typing fixtures/typing-override.pyi]
[builtins fixtures/staticmethod.pyi]

[case explicitOverrideClassmethod]
# flags: --python-version 3.12
Expand Down Expand Up @@ -2818,8 +2817,8 @@ class D(A):
def f(cls, x: str) -> str: pass # E: Argument 1 of "f" is incompatible with supertype "A"; supertype defines the argument type as "int" \
# N: This violates the Liskov substitution principle \
# N: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
[typing fixtures/typing-full.pyi]
[builtins fixtures/callable.pyi]
[typing fixtures/typing-override.pyi]
[builtins fixtures/classmethod.pyi]

[case explicitOverrideProperty]
# flags: --python-version 3.12
Expand Down Expand Up @@ -2853,8 +2852,8 @@ class D(A):
# N: str \
# N: Subclass: \
# N: int
[typing fixtures/typing-override.pyi]
[builtins fixtures/property.pyi]
[typing fixtures/typing-full.pyi]

[case explicitOverrideSettableProperty]
# flags: --python-version 3.12
Expand Down Expand Up @@ -2891,8 +2890,8 @@ class D(A):

@f.setter
def f(self, value: int) -> None: pass
[typing fixtures/typing-override.pyi]
[builtins fixtures/property.pyi]
[typing fixtures/typing-full.pyi]

[case invalidExplicitOverride]
# flags: --python-version 3.12
Expand All @@ -2907,8 +2906,7 @@ class A: pass
def g() -> None:
@override # E: "override" used with a non-method
def h(b: bool) -> int: pass
[typing fixtures/typing-full.pyi]
[builtins fixtures/tuple.pyi]
[typing fixtures/typing-override.pyi]

[case explicitOverrideSpecialMethods]
# flags: --python-version 3.12
Expand All @@ -2924,8 +2922,7 @@ class B(A):
class C:
@override
def __init__(self, a: int) -> None: pass
[typing fixtures/typing-full.pyi]
[builtins fixtures/tuple.pyi]
[typing fixtures/typing-override.pyi]

[case explicitOverrideFromExtensions]
from typing_extensions import override
Expand All @@ -2936,7 +2933,6 @@ class A:
class B(A):
@override
def f2(self, x: int) -> str: pass # E: Method "f2" is marked as an override, but no base method was found with this name
[typing fixtures/typing-full.pyi]
[builtins fixtures/tuple.pyi]

[case explicitOverrideOverloads]
Expand All @@ -2953,8 +2949,7 @@ class B(A):
def f2(self, x: str) -> str: pass
@override
def f2(self, x: int | str) -> str: pass
[typing fixtures/typing-full.pyi]
[builtins fixtures/tuple.pyi]
[typing fixtures/typing-override.pyi]

[case explicitOverrideNotOnOverloadsImplementation]
# flags: --python-version 3.12
Expand All @@ -2978,8 +2973,7 @@ class C(A):
@overload
def f(self, y: str) -> str: pass
def f(self, y: int | str) -> str: pass
[typing fixtures/typing-full.pyi]
[builtins fixtures/tuple.pyi]
[typing fixtures/typing-override.pyi]

[case explicitOverrideOnMultipleOverloads]
# flags: --python-version 3.12
Expand All @@ -3005,5 +2999,96 @@ class C(A):
def f(self, y: str) -> str: pass
@override
def f(self, y: int | str) -> str: pass
[typing fixtures/typing-full.pyi]
[builtins fixtures/tuple.pyi]
[typing fixtures/typing-override.pyi]

[case requireExplicitOverrideMethod]
# flags: --strict-override-decorator --python-version 3.12
from typing import override

class A:
def f(self, x: int) -> str: pass

class B(A):
@override
def f(self, y: int) -> str: pass

class C(A):
def f(self, y: int) -> str: pass # E: Method "f" is not marked as override but is overriding a method in class "__main__.A"

class D(B):
def f(self, y: int) -> str: pass # E: Method "f" is not marked as override but is overriding a method in class "__main__.B"
[typing fixtures/typing-override.pyi]
cdce8p marked this conversation as resolved.
Show resolved Hide resolved

[case requireExplicitOverrideSpecialMethod]
# flags: --strict-override-decorator --python-version 3.12
from typing import Self, override

# Don't require override decorator for __init__ and __new__
# See: https://github.com/python/typing/issues/1376
class A:
def __init__(self) -> None: pass
def __new__(cls) -> Self: pass
[typing fixtures/typing-override.pyi]

[case requireExplicitOverrideProperty]
# flags: --strict-override-decorator --python-version 3.12
from typing import override

class A:
@property
def prop(self) -> int: pass

class B(A):
@override
@property
def prop(self) -> int: pass

class C(A):
@property
def prop(self) -> int: pass # E: Method "prop" is not marked as override but is overriding a method in class "__main__.A"
[typing fixtures/typing-override.pyi]
[builtins fixtures/property.pyi]

[case requireExplicitOverrideOverload]
# flags: --strict-override-decorator --python-version 3.12
from typing import overload, override

class A:
@overload
def f(self, x: int) -> str: ...
@overload
def f(self, x: str) -> str: ...
def f(self, x): pass

class B(A):
@overload
def f(self, y: int) -> str: ...
@overload
def f(self, y: str) -> str: ...
@override
cdce8p marked this conversation as resolved.
Show resolved Hide resolved
def f(self, y): pass

class C(A):
@overload
def f(self, y: int) -> str: ...
@overload
def f(self, y: str) -> str: ...
def f(self, y): pass # E: Method "f" is not marked as override but is overriding a method in class "__main__.A"
[typing fixtures/typing-override.pyi]

[case requireExplicitOverrideMultipleInheritance]
# flags: --strict-override-decorator --python-version 3.12
from typing import override
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test using override imported from typing_extensions on an older Python version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rather not do that. Since none of our test fixtures use @override, we inadvertently get errors for those as well. That's also why I added a separate fixture for these test cases with just enough definitions and no accidental overrides. I would have to do the same just for the typing_extensions fixture.

There is also a test case for an unused @override already. As this PR doesn't change the decorator parsing, it probably fine to skip it.

[case explicitOverrideFromExtensions]
from typing_extensions import override
class A:
def f(self, x: int) -> str: pass
class B(A):
@override
def f2(self, x: int) -> str: pass # E: Method "f2" is marked as an override, but no base method was found with this name
[typing fixtures/typing-full.pyi]
[builtins fixtures/tuple.pyi]


class A:
def f(self, x: int) -> str: pass
class B:
def f(self, y: int) -> str: pass

class C(A, B):
@override
def f(self, z: int) -> str: pass

class D(A, B):
def f(self, z: int) -> str: pass # E: Method "f" is not marked as override but is overriding a method in class "__main__.A"
[typing fixtures/typing-override.pyi]
24 changes: 24 additions & 0 deletions test-data/unit/fixtures/typing-override.pyi
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
TypeVar = 0
Generic = 0
Any = 0
overload = 0
Type = 0
Literal = 0
Optional = 0
Self = 0
Tuple = 0
ClassVar = 0

T = TypeVar('T')
T_co = TypeVar('T_co', covariant=True)
KT = TypeVar('KT')

class Iterable(Generic[T_co]): pass
class Iterator(Iterable[T_co]): pass
class Sequence(Iterable[T_co]): pass
class Mapping(Iterable[KT], Generic[KT, T_co]):
def keys(self) -> Iterable[T]: pass # Approximate return type
def __getitem__(self, key: T) -> T_co: pass


def override(__arg: T) -> T: ...
Loading