Handle empty bodies safely (#13729)

Fixes #2350

This essentially re-applies #8111
modulo various (logical) changes in master since then.The only important
difference is that now I override few return-related error codes for
empty bodies, to allow opting out easily in next few versions (I still
keep the flag to simplify testing).

Author: ilevkivskyi

docs/source/class_basics.rst

@@ -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

docs/source/error_code_list.rst

@@ -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]
 -------------------------------------------
 

docs/source/protocols.rst

@@ -149,7 +149,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
 *******************

mypy/checker.py

@@ -63,13 +63,15 @@
     ARG_STAR,
     CONTRAVARIANT,
     COVARIANT,
+    FUNC_NO_INFO,
     GDEF,
     IMPLICITLY_ABSTRACT,
     INVARIANT,
     IS_ABSTRACT,
     LDEF,
     LITERAL_TYPE,
     MDEF,
+    NOT_ABSTRACT,
     AssertStmt,
     AssignmentExpr,
     AssignmentStmt,
@@ -620,7 +622,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)):
@@ -987,7 +989,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.
 
@@ -1001,7 +1007,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")
 
@@ -1018,7 +1024,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)
@@ -1190,7 +1198,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"
                 ):
@@ -1203,28 +1211,79 @@ 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
+                    )
+                )
+
+                # Ignore plugin generated methods, these usually don't need any bodies.
+                if defn.info is not FUNC_NO_INFO and (
+                    defn.name not in defn.info.names or defn.info.names[defn.name].plugin_generated
+                ):
+                    show_error = False
+
+                # Ignore also definitions that appear in `if TYPE_CHECKING: ...` blocks.
+                # These can't be called at runtime anyway (similar to plugin-generated).
+                if isinstance(defn, FuncDef) and defn.is_mypy_only:
+                    show_error = False
+
+                # We want to minimize the fallout from checking empty bodies
+                # that was absent in many mypy versions.
+                if body_is_trivial and is_subtype(NoneType(), return_type):
+                    show_error = False
+
+                may_be_abstract = (
+                    body_is_trivial
+                    and defn.info is not FUNC_NO_INFO
+                    and defn.info.metaclass_type is not None
+                    and defn.info.metaclass_type.type.has_base("abc.ABCMeta")
+                )
+
                 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.
+                        # declared to return a non-None type.
                         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)
+                        if may_be_abstract:
+                            self.note(message_registry.EMPTY_BODY_ABSTRACT, 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,
-                    )
+                    if (
+                        not self.check_subtype(
+                            subtype_label="implicitly returns",
+                            subtype=NoneType(),
+                            supertype_label="expected",
+                            supertype=return_type,
+                            context=defn,
+                            msg=msg,
+                        )
+                        and may_be_abstract
+                    ):
+                        self.note(message_registry.EMPTY_BODY_ABSTRACT, defn)
 
             self.return_types.pop()
 
@@ -6125,9 +6184,17 @@ def fail(
         self.msg.fail(msg, context, code=code)
 
     def note(
-        self, msg: str, context: Context, offset: int = 0, *, code: ErrorCode | None = None
+        self,
+        msg: str | ErrorMessage,
+        context: Context,
+        offset: int = 0,
+        *,
+        code: ErrorCode | None = None,
     ) -> None:
         """Produce a note."""
+        if isinstance(msg, ErrorMessage):
+            self.msg.note(msg.value, context, code=msg.code)
+            return
         self.msg.note(msg, context, offset=offset, code=code)
 
     def iterable_item_type(self, instance: Instance) -> Type:

mypy/checkmember.py

@@ -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])
@@ -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?
@@ -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

mypy/errorcodes.py

@@ -96,6 +96,16 @@ def __str__(self) -> str:
 UNUSED_COROUTINE: Final = ErrorCode(
     "unused-coroutine", "Ensure that all coroutines are used", "General"
 )
+# TODO: why do we need the explicit type here? Without it mypyc CI builds fail with
+# mypy/message_registry.py:37: error: Cannot determine type of "EMPTY_BODY"  [has-type]
+EMPTY_BODY: Final[ErrorCode] = 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(

mypy/main.py

@@ -999,6 +999,11 @@ def add_invertible_flag(
         "the contents of SHADOW_FILE instead.",
     )
     add_invertible_flag("--fast-exit", default=True, help=argparse.SUPPRESS, group=internals_group)
+    # This flag is useful for mypy tests, where function bodies may be omitted. Plugin developers
+    # may want to use this as well in their tests.
+    add_invertible_flag(
+        "--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."

mypy/message_registry.py

@@ -33,6 +33,9 @@ def with_additional_msg(self, info: str) -> ErrorMessage:
 # Type checker error message constants
 NO_RETURN_VALUE_EXPECTED: Final = ErrorMessage("No return value expected", codes.RETURN_VALUE)
 MISSING_RETURN_STATEMENT: Final = ErrorMessage("Missing return statement", codes.RETURN)
+EMPTY_BODY_ABSTRACT: Final = ErrorMessage(
+    "If the method is meant to be abstract, use @abc.abstractmethod", codes.EMPTY_BODY
+)
 INVALID_IMPLICIT_RETURN: Final = ErrorMessage("Implicit return in function which does not return")
 INCOMPATIBLE_RETURN_VALUE_TYPE: Final = ErrorMessage(
     "Incompatible return value type", codes.RETURN_VALUE

mypy/messages.py

@@ -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)
 

mypy/nodes.py

@@ -758,7 +758,12 @@ 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",
+    "is_mypy_only",
+]
 
 # Abstract status of a function
 NOT_ABSTRACT: Final = 0
@@ -781,6 +786,8 @@ class FuncDef(FuncItem, SymbolNode, Statement):
         "abstract_status",
         "original_def",
         "deco_line",
+        "is_trivial_body",
+        "is_mypy_only",
     )
 
     # Note that all __init__ args must have default values
@@ -796,11 +803,16 @@ 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
-        # Used for error reporting (to keep backwad compatibility with pre-3.8)
+        # Used for error reporting (to keep backward compatibility with pre-3.8)
         self.deco_line: int | None = None
+        # Definitions that appear in if TYPE_CHECKING are marked with this flag.
+        self.is_mypy_only = False
 
     @property
     def name(self) -> str: