Give error if __exit__ returns False but is declared to return bool (#7655)

Mypy can give false positives about missing return statements
if `__exit__` that always returns `False` is annotated to return
`bool` instead of `Literal[False]`.

Add new error code and documentation for the error code since
this error condition is not very obvious.

Fixes #7577.

There are two major limitations:

1. This doesn't support async context managers.
2. This won't help if a stub has an invalid `__exit__` return type.

I'll create a follow-up issues about the above.

Author: JukkaL

docs/source/error_code_list.rst

@@ -593,6 +593,63 @@ To work around the issue, you can either give mypy access to the sources
 for ``acme`` or create a stub file for the module.  See :ref:`ignore-missing-imports`
 for more information.
 
+Check the return type of __exit__ [exit-return]
+-----------------------------------------------
+
+If mypy can determine that ``__exit__`` always returns ``False``, mypy
+checks that the return type is *not* ``bool``.  The boolean value of
+the return type affects which lines mypy thinks are reachable after a
+``with`` statement, since any ``__exit__`` method that can return
+``True`` may swallow exceptions. An imprecise return type can result
+in mysterious errors reported near ``with`` statements.
+
+To fix this, use either ``typing_extensions.Literal[False]`` or
+``None`` as the return type. Returning ``None`` is equivalent to
+returning ``False`` in this context, since both are treated as false
+values.
+
+Example:
+
+.. code-block:: python
+
+   class MyContext:
+       ...
+       def __exit__(self, exc, value, tb) -> bool:  # Error
+           print('exit')
+           return False
+
+This produces the following output from mypy:
+
+.. code-block:: text
+
+   example.py:3: error: "bool" is invalid as return type for "__exit__" that always returns False
+   example.py:3: note: Use "typing_extensions.Literal[False]" as the return type or change it to
+       "None"
+   example.py:3: note: If return type of "__exit__" implies that it may return True, the context
+       manager may swallow exceptions
+
+You can use ``Literal[False]`` to fix the error:
+
+.. code-block:: python
+
+   from typing_extensions import Literal
+
+   class MyContext:
+       ...
+       def __exit__(self, exc, value, tb) -> Literal[False]:  # OK
+           print('exit')
+           return False
+
+You can also use ``None``:
+
+.. code-block:: python
+
+   class MyContext:
+       ...
+       def __exit__(self, exc, value, tb) -> None:  # Also OK
+           print('exit')
+
+
 Report syntax errors [syntax]
 -----------------------------
 

mypy/checker.py

@@ -76,7 +76,7 @@
 from mypy.scope import Scope
 from mypy.typeops import tuple_fallback
 from mypy import state, errorcodes as codes
-from mypy.traverser import has_return_statement
+from mypy.traverser import has_return_statement, all_return_statements
 from mypy.errorcodes import ErrorCode
 
 T = TypeVar('T')
@@ -791,6 +791,9 @@ def check_func_item(self, defn: FuncItem,
         self.dynamic_funcs.pop()
         self.current_node_deferred = False
 
+        if name == '__exit__':
+            self.check__exit__return_type(defn)
+
     @contextmanager
     def enter_attribute_inference_context(self) -> Iterator[None]:
         old_types = self.inferred_attribute_types
@@ -1667,6 +1670,29 @@ def erase_override(t: Type) -> Type:
                 self.note("Overloaded operator methods can't have wider argument types"
                           " in overrides", node, code=codes.OVERRIDE)
 
+    def check__exit__return_type(self, defn: FuncItem) -> None:
+        """Generate error if the return type of __exit__ is problematic.
+
+        If __exit__ always returns False but the return type is declared
+        as bool, mypy thinks that a with statement may "swallow"
+        exceptions even though this is not the case, resulting in
+        invalid reachability inference.
+        """
+        if not defn.type or not isinstance(defn.type, CallableType):
+            return
+
+        ret_type = get_proper_type(defn.type.ret_type)
+        if not has_bool_item(ret_type):
+            return
+
+        returns = all_return_statements(defn)
+        if not returns:
+            return
+
+        if all(isinstance(ret.expr, NameExpr) and ret.expr.fullname == 'builtins.False'
+               for ret in returns):
+            self.msg.incorrect__exit__return(defn)
+
     def visit_class_def(self, defn: ClassDef) -> None:
         """Type check a class definition."""
         typ = defn.info
@@ -4793,3 +4819,13 @@ def coerce_to_literal(typ: Type) -> ProperType:
         return typ.last_known_value
     else:
         return typ
+
+
+def has_bool_item(typ: ProperType) -> bool:
+    """Return True if type is 'bool' or a union with a 'bool' item."""
+    if is_named_instance(typ, 'builtins.bool'):
+        return True
+    if isinstance(typ, UnionType):
+        return any(is_named_instance(item, 'builtins.bool')
+                   for item in typ.items)
+    return False

mypy/errorcodes.py

@@ -83,8 +83,10 @@ def __str__(self) -> str:
 STR_BYTES_PY3 = ErrorCode(
     'str-bytes-safe', "Warn about dangerous coercions related to bytes and string types",
     'General')  # type: Final
+EXIT_RETURN = ErrorCode(
+    'exit-return', "Warn about too general return type for '__exit__'", 'General')  # type: Final
 
-# These error codes aren't enable by default.
+# These error codes aren't enabled by default.
 NO_UNTYPED_DEF = ErrorCode(
     'no-untyped-def', "Check that every function has an annotation", 'General')  # type: Final
 NO_UNTYPED_CALL = ErrorCode(

mypy/ipc.py

@@ -169,9 +169,8 @@ def __exit__(self,
                  exc_ty: 'Optional[Type[BaseException]]' = None,
                  exc_val: Optional[BaseException] = None,
                  exc_tb: Optional[TracebackType] = None,
-                 ) -> bool:
+                 ) -> None:
         self.close()
-        return False
 
 
 class IPCServer(IPCBase):
@@ -246,7 +245,7 @@ def __exit__(self,
                  exc_ty: 'Optional[Type[BaseException]]' = None,
                  exc_val: Optional[BaseException] = None,
                  exc_tb: Optional[TracebackType] = None,
-                 ) -> bool:
+                 ) -> None:
         if sys.platform == 'win32':
             try:
                 # Wait for the client to finish reading the last write before disconnecting
@@ -257,7 +256,6 @@ def __exit__(self,
                 DisconnectNamedPipe(self.connection)
         else:
             self.close()
-        return False
 
     def cleanup(self) -> None:
         if sys.platform == 'win32':

mypy/messages.py

@@ -1104,6 +1104,18 @@ def incorrectly_returning_any(self, typ: Type, context: Context) -> None:
             format_type(typ))
         self.fail(message, context, code=codes.NO_ANY_RETURN)
 
+    def incorrect__exit__return(self, context: Context) -> None:
+        self.fail(
+            '"bool" is invalid as return type for "__exit__" that always returns False', context,
+            code=codes.EXIT_RETURN)
+        self.note(
+            'Use "typing_extensions.Literal[False]" as the return type or change it to "None"',
+            context, code=codes.EXIT_RETURN)
+        self.note(
+            'If return type of "__exit__" implies that it may return True, '
+            'the context manager may swallow exceptions',
+            context, code=codes.EXIT_RETURN)
+
     def untyped_decorated_function(self, typ: Type, context: Context) -> None:
         typ = get_proper_type(typ)
         if isinstance(typ, AnyType):

mypy/traverser.py

@@ -1,5 +1,7 @@
 """Generic node traverser visitor"""
 
+from typing import List
+
 from mypy.visitor import NodeVisitor
 from mypy.nodes import (
     Block, MypyFile, FuncBase, FuncItem, CallExpr, ClassDef, Decorator, FuncDef,
@@ -10,7 +12,7 @@
     GeneratorExpr, ListComprehension, SetComprehension, DictionaryComprehension,
     ConditionalExpr, TypeApplication, ExecStmt, Import, ImportFrom,
     LambdaExpr, ComparisonExpr, OverloadedFuncDef, YieldFromExpr,
-    YieldExpr, StarExpr, BackquoteExpr, AwaitExpr, PrintStmt, SuperExpr, REVEAL_TYPE,
+    YieldExpr, StarExpr, BackquoteExpr, AwaitExpr, PrintStmt, SuperExpr, Node, REVEAL_TYPE,
 )
 
 
@@ -309,3 +311,24 @@ def has_return_statement(fdef: FuncBase) -> bool:
     seeker = ReturnSeeker()
     fdef.accept(seeker)
     return seeker.found
+
+
+class ReturnCollector(TraverserVisitor):
+    def __init__(self) -> None:
+        self.return_statements = []  # type: List[ReturnStmt]
+        self.inside_func = False
+
+    def visit_func_def(self, defn: FuncDef) -> None:
+        if not self.inside_func:
+            self.inside_func = True
+            super().visit_func_def(defn)
+            self.inside_func = False
+
+    def visit_return_stmt(self, stmt: ReturnStmt) -> None:
+        self.return_statements.append(stmt)
+
+
+def all_return_statements(node: Node) -> List[ReturnStmt]:
+    v = ReturnCollector()
+    node.accept(v)
+    return v.return_statements

test-data/stdlib-samples/3.2/subprocess.py

@@ -351,6 +351,7 @@ class Popen(args, bufsize=0, executable=None,
     Any, Tuple, List, Sequence, Callable, Mapping, cast, Set, Dict, IO,
     TextIO, AnyStr
 )
+from typing_extensions import Literal
 from types import TracebackType
 
 # Exception classes used by this module.
@@ -775,7 +776,7 @@ def __enter__(self) -> 'Popen':
         return self
 
     def __exit__(self, type: type, value: BaseException,
-                 traceback: TracebackType) -> bool:
+                 traceback: TracebackType) -> Literal[False]:
         if self.stdout:
             self.stdout.close()
         if self.stderr:

test-data/stdlib-samples/3.2/tempfile.py

@@ -41,6 +41,7 @@
     List as _List, Tuple as _Tuple, Dict as _Dict, Iterable as _Iterable,
     IO as _IO, cast as _cast, Optional as _Optional, Type as _Type,
 )
+from typing_extensions import Literal
 from types import TracebackType as _TracebackType
 
 try:
@@ -419,8 +420,10 @@ def __exit__(self, exc: _Type[BaseException], value: BaseException,
             self.close()
             return result
     else:
-        def __exit__(self, exc: _Type[BaseException], value: BaseException,
-                     tb: _Optional[_TracebackType]) -> bool:
+        def __exit__(self,  # type: ignore[misc]
+                     exc: _Type[BaseException],
+                     value: BaseException,
+                     tb: _Optional[_TracebackType]) -> Literal[False]:
             self.file.__exit__(exc, value, tb)
             return False
 
@@ -554,7 +557,7 @@ def __enter__(self) -> 'SpooledTemporaryFile':
         return self
 
     def __exit__(self, exc: type, value: BaseException,
-                 tb: _TracebackType) -> bool:
+                 tb: _TracebackType) -> Literal[False]:
         self._file.close()
         return False
 
@@ -691,7 +694,7 @@ def cleanup(self, _warn: bool = False) -> None:
                            ResourceWarning)
 
     def __exit__(self, exc: type, value: BaseException,
-                 tb: _TracebackType) -> bool:
+                 tb: _TracebackType) -> Literal[False]:
         self.cleanup()
         return False
 

test-data/unit/check-errorcodes.test

@@ -695,3 +695,11 @@ x = y  # type: int  # type: ignored [foo]
 [out]
 main:1: error: syntax error in type comment 'int'  [syntax]
 main:2: error: syntax error in type comment 'int'  [syntax]
+
+[case testErrorCode__exit__Return]
+class InvalidReturn:
+    def __exit__(self, x, y, z) -> bool:  # E: "bool" is invalid as return type for "__exit__" that always returns False  [exit-return] \
+# N: Use "typing_extensions.Literal[False]" as the return type or change it to "None" \
+# N: If return type of "__exit__" implies that it may return True, the context manager may swallow exceptions
+        return False
+[builtins fixtures/bool.pyi]

test-data/unit/check-statements.test

@@ -1443,6 +1443,97 @@ with A(), B(), B() as p, A(), A():  # type: str
     pass
 [builtins fixtures/tuple.pyi]
 
+[case testWithStmtBoolExitReturnWithResultFalse]
+from typing import Optional
+
+class InvalidReturn1:
+    def __exit__(self, x, y, z) -> bool:  # E: "bool" is invalid as return type for "__exit__" that always returns False \
+# N: Use "typing_extensions.Literal[False]" as the return type or change it to "None" \
+# N: If return type of "__exit__" implies that it may return True, the context manager may swallow exceptions
+        return False
+
+class InvalidReturn2:
+    def __exit__(self, x, y, z) -> Optional[bool]:  # E: "bool" is invalid as return type for "__exit__" that always returns False \
+# N: Use "typing_extensions.Literal[False]" as the return type or change it to "None" \
+# N: If return type of "__exit__" implies that it may return True, the context manager may swallow exceptions
+        if int():
+            return False
+        else:
+            return False
+
+class InvalidReturn3:
+    def __exit__(self, x, y, z) -> bool:  # E: "bool" is invalid as return type for "__exit__" that always returns False \
+# N: Use "typing_extensions.Literal[False]" as the return type or change it to "None" \
+# N: If return type of "__exit__" implies that it may return True, the context manager may swallow exceptions
+        def nested() -> bool:
+            return True
+        return False
+[builtins fixtures/bool.pyi]
+
+[case testWithStmtBoolExitReturnOkay]
+from typing_extensions import Literal
+
+class GoodReturn1:
+    def __exit__(self, x, y, z) -> bool:
+        if int():
+            return True
+        else:
+            return False
+
+class GoodReturn2:
+    def __exit__(self, x, y, z) -> bool:
+        if int():
+            return False
+        else:
+            return True
+
+class GoodReturn3:
+    def __exit__(self, x, y, z) -> bool:
+        return bool()
+
+class GoodReturn4:
+    def __exit__(self, x, y, z) -> None:
+        return
+
+class GoodReturn5:
+    def __exit__(self, x, y, z) -> None:
+        return None
+
+class GoodReturn6:
+    def exit(self, x, y, z) -> bool:
+        return False
+
+class GoodReturn7:
+    def exit(self, x, y, z) -> bool:
+        pass
+
+class MissingReturn:
+    def exit(self, x, y, z) -> bool: # E: Missing return statement
+        x = 0
+
+class LiteralReturn:
+    def __exit__(self, x, y, z) -> Literal[False]:
+        return False
+[builtins fixtures/bool.pyi]
+
+
+[case testWithStmtBoolExitReturnInStub]
+import stub
+
+[file stub.pyi]
+from typing import Optional
+
+class C1:
+    def __exit__(self, x, y, z) -> bool: ...
+
+class C2:
+    def __exit__(self, x, y, z) -> bool: pass
+
+class C3:
+    def __exit__(self, x, y, z) -> Optional[bool]: pass
+[builtins fixtures/bool.pyi]
+
+
 -- Chained assignment
 -- ------------------