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

Handle empty bodies safely #13729

Merged
merged 14 commits into from
Sep 29, 2022
20 changes: 20 additions & 0 deletions docs/source/class_basics.rst
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,26 @@ however:
in this case, but any attempt to construct an instance will be
flagged as an error.

Mypy allows you to omit the body for an abstract method, but if you do so,
it is unsafe to call such method via ``super()``. For example:

.. code-block:: python

from abc import abstractmethod
class Base:
@abstractmethod
def foo(self) -> int: pass
@abstractmethod
def bar(self) -> int:
return 0
class Sub(Base):
def foo(self) -> int:
return super().foo() + 1 # error: Call to abstract method "foo" of "Base"
# with trivial body via super() is unsafe
@abstractmethod
def bar(self) -> int:
return super().bar() + 1 # This is OK however.

A class can inherit any number of classes, both abstract and
concrete. As with normal overrides, a dynamically typed method can
override or implement a statically typed method defined in any base
Expand Down
22 changes: 22 additions & 0 deletions docs/source/error_code_list.rst
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,28 @@ Example:
# Error: Cannot instantiate abstract class "Thing" with abstract attribute "save" [abstract]
t = Thing()

Check that call to an abstract method via super is valid [safe-super]
---------------------------------------------------------------------

Abstract methods often don't have any default implementation, i.e. their
bodies are just empty. Calling such methods in subclasses via ``super()``
will cause runtime errors, so mypy prevents you from doing so:

.. code-block:: python

from abc import abstractmethod
class Base:
@abstractmethod
def foo(self) -> int: ...
class Sub(Base):
def foo(self) -> int:
return super().foo() + 1 # error: Call to abstract method "foo" of "Base" with
# trivial body via super() is unsafe [safe-super]
Sub().foo() # This will crash at runtime.

Mypy considers the following as trivial bodies: a ``pass`` statement, a literal
ellipsis ``...``, a docstring, and a ``raise NotImplementedError`` statement.

Check the target of NewType [valid-newtype]
-------------------------------------------

Expand Down
14 changes: 13 additions & 1 deletion docs/source/protocols.rst
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,19 @@ protocols. If you explicitly subclass these protocols you can inherit
these default implementations. Explicitly including a protocol as a
base class is also a way of documenting that your class implements a
particular protocol, and it forces mypy to verify that your class
implementation is actually compatible with the protocol.
implementation is actually compatible with the protocol. In particular,
omitting a value for an attribute or a method body will make it implicitly
abstract:

.. code-block:: python

class SomeProto(Protocol):
attr: int # Note, no right hand side
def method(self) -> str: ... # Literal ... here
class ExplicitSubclass(SomeProto):
pass
ExplicitSubclass() # error: Cannot instantiate abstract class 'ExplicitSubclass'
# with abstract attributes 'attr' and 'method'

Recursive protocols
*******************
Expand Down
48 changes: 37 additions & 11 deletions mypy/checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
LDEF,
LITERAL_TYPE,
MDEF,
NOT_ABSTRACT,
AssertStmt,
AssignmentExpr,
AssignmentStmt,
Expand Down Expand Up @@ -620,7 +621,7 @@ def _visit_overloaded_func_def(self, defn: OverloadedFuncDef) -> None:
self.visit_decorator(cast(Decorator, defn.items[0]))
for fdef in defn.items:
assert isinstance(fdef, Decorator)
self.check_func_item(fdef.func, name=fdef.func.name)
self.check_func_item(fdef.func, name=fdef.func.name, allow_empty=True)
if fdef.func.abstract_status in (IS_ABSTRACT, IMPLICITLY_ABSTRACT):
num_abstract += 1
if num_abstract not in (0, len(defn.items)):
Expand Down Expand Up @@ -986,7 +987,11 @@ def _visit_func_def(self, defn: FuncDef) -> None:
)

def check_func_item(
self, defn: FuncItem, type_override: CallableType | None = None, name: str | None = None
self,
defn: FuncItem,
type_override: CallableType | None = None,
name: str | None = None,
allow_empty: bool = False,
) -> None:
"""Type check a function.

Expand All @@ -1000,7 +1005,7 @@ def check_func_item(
typ = type_override.copy_modified(line=typ.line, column=typ.column)
if isinstance(typ, CallableType):
with self.enter_attribute_inference_context():
self.check_func_def(defn, typ, name)
self.check_func_def(defn, typ, name, allow_empty)
else:
raise RuntimeError("Not supported")

Expand All @@ -1017,7 +1022,9 @@ def enter_attribute_inference_context(self) -> Iterator[None]:
yield None
self.inferred_attribute_types = old_types

def check_func_def(self, defn: FuncItem, typ: CallableType, name: str | None) -> None:
def check_func_def(
self, defn: FuncItem, typ: CallableType, name: str | None, allow_empty: bool = False
) -> None:
"""Type check a function definition."""
# Expand type variables with value restrictions to ordinary types.
expanded = self.expand_typevars(defn, typ)
Expand Down Expand Up @@ -1189,7 +1196,7 @@ def check_func_def(self, defn: FuncItem, typ: CallableType, name: str | None) ->
self.accept(item.body)
unreachable = self.binder.is_unreachable()

if not unreachable and not body_is_trivial:
if not unreachable:
if defn.is_generator or is_named_instance(
self.return_types[-1], "typing.AwaitableGenerator"
):
Expand All @@ -1202,27 +1209,46 @@ def check_func_def(self, defn: FuncItem, typ: CallableType, name: str | None) ->
return_type = self.return_types[-1]
return_type = get_proper_type(return_type)

allow_empty = allow_empty or self.options.allow_empty_bodies

show_error = (
not body_is_trivial
or
# Allow empty bodies for abstract methods, overloads, in tests and stubs.
not allow_empty
and not (isinstance(defn, FuncDef) and defn.abstract_status != NOT_ABSTRACT)
and not self.is_stub
ilevkivskyi marked this conversation as resolved.
Show resolved Hide resolved
)

if self.options.warn_no_return:
if not self.current_node_deferred and not isinstance(
return_type, (NoneType, AnyType)
if (
not self.current_node_deferred
and not isinstance(return_type, (NoneType, AnyType))
and show_error
):
# Control flow fell off the end of a function that was
# declared to return a non-None type and is not
# entirely pass/Ellipsis/raise NotImplementedError.
ilevkivskyi marked this conversation as resolved.
Show resolved Hide resolved
if isinstance(return_type, UninhabitedType):
# This is a NoReturn function
self.fail(message_registry.INVALID_IMPLICIT_RETURN, defn)
msg = message_registry.INVALID_IMPLICIT_RETURN
else:
self.fail(message_registry.MISSING_RETURN_STATEMENT, defn)
else:
msg = message_registry.MISSING_RETURN_STATEMENT
if body_is_trivial:
msg = msg._replace(code=codes.EMPTY_BODY)
self.fail(msg, defn)
elif show_error:
msg = message_registry.INCOMPATIBLE_RETURN_VALUE_TYPE
if body_is_trivial:
msg = msg._replace(code=codes.EMPTY_BODY)
# similar to code in check_return_stmt
self.check_subtype(
subtype_label="implicitly returns",
subtype=NoneType(),
supertype_label="expected",
supertype=return_type,
context=defn,
msg=message_registry.INCOMPATIBLE_RETURN_VALUE_TYPE,
msg=msg,
)

self.return_types.pop()
Expand Down
24 changes: 24 additions & 0 deletions mypy/checkmember.py
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,9 @@ def analyze_instance_member_access(
# Look up the member. First look up the method dictionary.
method = info.get_method(name)
if method and not isinstance(method, Decorator):
if mx.is_super:
validate_super_call(method, mx)

if method.is_property:
assert isinstance(method, OverloadedFuncDef)
first_item = cast(Decorator, method.items[0])
Expand Down Expand Up @@ -328,6 +331,25 @@ def analyze_instance_member_access(
return analyze_member_var_access(name, typ, info, mx)


def validate_super_call(node: FuncBase, mx: MemberContext) -> None:
unsafe_super = False
if isinstance(node, FuncDef) and node.is_trivial_body:
unsafe_super = True
impl = node
elif isinstance(node, OverloadedFuncDef):
if node.impl:
impl = node.impl if isinstance(node.impl, FuncDef) else node.impl.func
unsafe_super = impl.is_trivial_body
if unsafe_super:
ret_type = (
impl.type.ret_type
if isinstance(impl.type, CallableType)
else AnyType(TypeOfAny.unannotated)
)
if not subtypes.is_subtype(NoneType(), ret_type):
mx.msg.unsafe_super(node.name, node.info.name, mx.context)


def analyze_type_callable_member_access(name: str, typ: FunctionLike, mx: MemberContext) -> Type:
# Class attribute.
# TODO super?
Expand Down Expand Up @@ -449,6 +471,8 @@ def analyze_member_var_access(
if isinstance(vv, Decorator):
# The associated Var node of a decorator contains the type.
v = vv.var
if mx.is_super:
validate_super_call(vv.func, mx)

if isinstance(vv, TypeInfo):
# If the associated variable is a TypeInfo synthesize a Var node for
Expand Down
8 changes: 8 additions & 0 deletions mypy/errorcodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,14 @@ def __str__(self) -> str:
UNUSED_COROUTINE: Final = ErrorCode(
"unused-coroutine", "Ensure that all coroutines are used", "General"
)
EMPTY_BODY: Final = ErrorCode(
"empty-body",
"A dedicated error code to opt out return errors for empty/trivial bodies",
"General",
)
SAFE_SUPER: Final = ErrorCode(
"safe-super", "Warn about calls to abstract methods with empty/trivial bodies", "General"
)

# These error codes aren't enabled by default.
NO_UNTYPED_DEF: Final[ErrorCode] = ErrorCode(
Expand Down
3 changes: 3 additions & 0 deletions mypy/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -999,6 +999,9 @@ def add_invertible_flag(
"the contents of SHADOW_FILE instead.",
)
add_invertible_flag("--fast-exit", default=True, help=argparse.SUPPRESS, group=internals_group)
add_invertible_flag(
ilevkivskyi marked this conversation as resolved.
Show resolved Hide resolved
"--allow-empty-bodies", default=False, help=argparse.SUPPRESS, group=internals_group
)

report_group = parser.add_argument_group(
title="Report generation", description="Generate a report in the specified format."
Expand Down
8 changes: 8 additions & 0 deletions mypy/messages.py
Original file line number Diff line number Diff line change
Expand Up @@ -1231,6 +1231,14 @@ def first_argument_for_super_must_be_type(self, actual: Type, context: Context)
code=codes.ARG_TYPE,
)

def unsafe_super(self, method: str, cls: str, ctx: Context) -> None:
self.fail(
'Call to abstract method "{}" of "{}" with trivial body'
" via super() is unsafe".format(method, cls),
ctx,
code=codes.SAFE_SUPER,
)

def too_few_string_formatting_arguments(self, context: Context) -> None:
self.fail("Not enough arguments for format string", context, code=codes.STRING_FORMATTING)

Expand Down
6 changes: 5 additions & 1 deletion mypy/nodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -758,7 +758,7 @@ def is_dynamic(self) -> bool:
return self.type is None


FUNCDEF_FLAGS: Final = FUNCITEM_FLAGS + ["is_decorated", "is_conditional"]
FUNCDEF_FLAGS: Final = FUNCITEM_FLAGS + ["is_decorated", "is_conditional", "is_trivial_body"]

# Abstract status of a function
NOT_ABSTRACT: Final = 0
Expand All @@ -781,6 +781,7 @@ class FuncDef(FuncItem, SymbolNode, Statement):
"abstract_status",
"original_def",
"deco_line",
"is_trivial_body",
)

# Note that all __init__ args must have default values
Expand All @@ -796,6 +797,9 @@ def __init__(
self.is_decorated = False
self.is_conditional = False # Defined conditionally (within block)?
self.abstract_status = NOT_ABSTRACT
# Is this an abstract method with trivial body?
# Such methods can't be called via super().
self.is_trivial_body = False
self.is_final = False
# Original conditional definition
self.original_def: None | FuncDef | Var | Decorator = None
Expand Down
2 changes: 2 additions & 0 deletions mypy/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,8 @@ def __init__(self) -> None:
self.fast_exit = True
# fast path for finding modules from source set
self.fast_module_lookup = False
# Allow empty function bodies even if it is not safe, used for testing only.
self.allow_empty_bodies = False
# Used to transform source code before parsing if not None
# TODO: Make the type precise (AnyStr -> AnyStr)
self.transform_source: Callable[[Any], Any] | None = None
Expand Down
10 changes: 10 additions & 0 deletions mypy/semanal.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@
IS_ABSTRACT,
LDEF,
MDEF,
NOT_ABSTRACT,
REVEAL_LOCALS,
REVEAL_TYPE,
RUNTIME_PROTOCOL_DECOS,
Expand Down Expand Up @@ -861,6 +862,12 @@ def analyze_func_def(self, defn: FuncDef) -> None:
and is_trivial_body(defn.body)
):
defn.abstract_status = IMPLICITLY_ABSTRACT
if (
is_trivial_body(defn.body)
and not self.is_stub_file
and defn.abstract_status != NOT_ABSTRACT
):
defn.is_trivial_body = True

if (
defn.is_coroutine
Expand Down Expand Up @@ -1038,6 +1045,8 @@ def process_overload_impl(self, defn: OverloadedFuncDef) -> None:
assert self.type is not None
if self.type.is_protocol:
impl.abstract_status = IMPLICITLY_ABSTRACT
if impl.abstract_status != NOT_ABSTRACT:
impl.is_trivial_body = True

def analyze_overload_sigs_and_impl(
self, defn: OverloadedFuncDef
Expand Down Expand Up @@ -1125,6 +1134,7 @@ def handle_missing_overload_implementation(self, defn: OverloadedFuncDef) -> Non
else:
item.abstract_status = IS_ABSTRACT
else:
# TODO: also allow omitting an implementation for abstract methods in ABCs?
self.fail(
"An overloaded function outside a stub file must have an implementation",
defn,
Expand Down
8 changes: 8 additions & 0 deletions mypy/server/astdiff.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ class level -- these are handled at attribute level (say, 'mod.Cls.method'
UNBOUND_IMPORTED,
Decorator,
FuncBase,
FuncDef,
FuncItem,
MypyFile,
OverloadedFuncDef,
Expand Down Expand Up @@ -217,6 +218,12 @@ def snapshot_definition(node: SymbolNode | None, common: tuple[object, ...]) ->
signature = snapshot_type(node.type)
else:
signature = snapshot_untyped_signature(node)
impl: FuncDef | None = None
if isinstance(node, FuncDef):
impl = node
elif isinstance(node, OverloadedFuncDef) and node.impl:
impl = node.impl.func if isinstance(node.impl, Decorator) else node.impl
is_trivial_body = impl.is_trivial_body if impl else False
return (
"Func",
common,
Expand All @@ -225,6 +232,7 @@ def snapshot_definition(node: SymbolNode | None, common: tuple[object, ...]) ->
node.is_class,
node.is_static,
signature,
is_trivial_body,
)
elif isinstance(node, Var):
return ("Var", common, snapshot_optional_type(node.type), node.is_final)
Expand Down
2 changes: 2 additions & 0 deletions mypy/test/testcheck.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,8 @@ def run_case_once(
options.show_column_numbers = True
if "errorcodes" in testcase.file:
options.show_error_codes = True
if "abstract" not in testcase.file:
options.allow_empty_bodies = not testcase.name.endswith("_no_empty")

if incremental_step and options.incremental:
# Don't overwrite # flags: --no-incremental in incremental test cases
Expand Down
2 changes: 2 additions & 0 deletions mypy/test/testcmdline.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ def test_python_cmdline(testcase: DataDrivenTestCase, step: int) -> None:
args.append("--show-traceback")
if "--error-summary" not in args:
args.append("--no-error-summary")
if "--disallow-empty-bodies" not in args:
args.append("--allow-empty-bodies")
# Type check the program.
fixed = [python3_path, "-m", "mypy"]
env = os.environ.copy()
Expand Down
Loading