From 78885de0ded1633719531304650c4580ff4264c0 Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Fri, 4 Oct 2019 18:17:13 +0100 Subject: [PATCH 1/8] Basic implementation --- mypy/checker.py | 35 +++++++++++++++- mypy/messages.py | 11 +++++ mypy/traverser.py | 18 +++++++- test-data/unit/check-statements.test | 63 ++++++++++++++++++++++++++++ 4 files changed, 125 insertions(+), 2 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index b56e6a867d4ed..87a1b14cb9a51 100644 --- a/mypy/checker.py +++ b/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,26 @@ 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 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 +4816,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 diff --git a/mypy/messages.py b/mypy/messages.py index 1a947d80bf006..887035a7c8c8c 100644 --- a/mypy/messages.py +++ b/mypy/messages.py @@ -1104,6 +1104,17 @@ 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) + self.note( + 'Use "typing_extensions.Literal[False]" as the return type or change it to "None"', + context) + self.note( + 'If return type of "__exit__" implies that it may return True, ' + 'the context manager may swallow exceptions', + context) + def untyped_decorated_function(self, typ: Type, context: Context) -> None: typ = get_proper_type(typ) if isinstance(typ, AnyType): diff --git a/mypy/traverser.py b/mypy/traverser.py index b0619872e8868..5cf6d2db5c0ce 100644 --- a/mypy/traverser.py +++ b/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,17 @@ 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] + + 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 diff --git a/test-data/unit/check-statements.test b/test-data/unit/check-statements.test index 8f12be62d653b..f747498055042 100644 --- a/test-data/unit/check-statements.test +++ b/test-data/unit/check-statements.test @@ -1443,6 +1443,69 @@ with A(), B(), B() as p, A(), A(): # type: str pass [builtins fixtures/tuple.pyi] +[case testWithStmtBoolExitReturnWithResultFalse] +from typing import Optional +from typing_extensions import Literal + +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 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] + + -- Chained assignment -- ------------------ From 9a28dd478a369a9e3fb7ab26aeb5bfeba8a17284 Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Mon, 7 Oct 2019 17:52:39 +0100 Subject: [PATCH 2/8] Handle nested functions --- mypy/traverser.py | 7 +++++++ test-data/unit/check-statements.test | 13 ++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/mypy/traverser.py b/mypy/traverser.py index 5cf6d2db5c0ce..de43269f31a95 100644 --- a/mypy/traverser.py +++ b/mypy/traverser.py @@ -316,6 +316,13 @@ def has_return_statement(fdef: FuncBase) -> bool: 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) diff --git a/test-data/unit/check-statements.test b/test-data/unit/check-statements.test index f747498055042..65ad9d6121e92 100644 --- a/test-data/unit/check-statements.test +++ b/test-data/unit/check-statements.test @@ -1445,7 +1445,6 @@ with A(), B(), B() as p, A(), A(): # type: str [case testWithStmtBoolExitReturnWithResultFalse] from typing import Optional -from typing_extensions import Literal class InvalidReturn1: def __exit__(self, x, y, z) -> bool: # E: "bool" is invalid as return type for "__exit__" that always returns False \ @@ -1462,6 +1461,18 @@ class InvalidReturn2: 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(): From 015de60548f345f80b267521bd95a5fac4ca5f3e Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Tue, 8 Oct 2019 13:04:44 +0100 Subject: [PATCH 3/8] Fix type check errors --- mypy/ipc.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/mypy/ipc.py b/mypy/ipc.py index bbfc1122bcc67..02c70eb82829c 100644 --- a/mypy/ipc.py +++ b/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': From 7c25979679989258128b8123dc97ed1e4fd445e5 Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Tue, 8 Oct 2019 13:39:58 +0100 Subject: [PATCH 4/8] Fix stubs --- mypy/checker.py | 3 +++ test-data/unit/check-statements.test | 17 +++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/mypy/checker.py b/mypy/checker.py index 87a1b14cb9a51..cb6242008e3d0 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -1686,6 +1686,9 @@ def check__exit__return_type(self, defn: FuncItem) -> None: 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) diff --git a/test-data/unit/check-statements.test b/test-data/unit/check-statements.test index 65ad9d6121e92..b8c8022c67340 100644 --- a/test-data/unit/check-statements.test +++ b/test-data/unit/check-statements.test @@ -1517,6 +1517,23 @@ class LiteralReturn: [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 -- ------------------ From 52380da304876f00b4ec7bd1f8c4b05777de8e0c Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Tue, 8 Oct 2019 14:12:20 +0100 Subject: [PATCH 5/8] Add error code for the new messages --- mypy/errorcodes.py | 4 +++- mypy/messages.py | 7 ++++--- test-data/unit/check-errorcodes.test | 8 ++++++++ 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/mypy/errorcodes.py b/mypy/errorcodes.py index 492e58fae05f9..7749db5e80088 100644 --- a/mypy/errorcodes.py +++ b/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( diff --git a/mypy/messages.py b/mypy/messages.py index 887035a7c8c8c..8eff1e6ebcbd0 100644 --- a/mypy/messages.py +++ b/mypy/messages.py @@ -1106,14 +1106,15 @@ def incorrectly_returning_any(self, typ: Type, context: Context) -> None: def incorrect__exit__return(self, context: Context) -> None: self.fail( - '"bool" is invalid as return type for "__exit__" that always returns False', context) + '"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) + 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) + context, code=codes.EXIT_RETURN) def untyped_decorated_function(self, typ: Type, context: Context) -> None: typ = get_proper_type(typ) diff --git a/test-data/unit/check-errorcodes.test b/test-data/unit/check-errorcodes.test index ce2f3de346874..04342c872d911 100644 --- a/test-data/unit/check-errorcodes.test +++ b/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] From 9e4c2ea01ff18aab69d5dfd889fe332ec6a00456 Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Tue, 8 Oct 2019 14:12:38 +0100 Subject: [PATCH 6/8] Fix stdlib samples test failure --- test-data/stdlib-samples/3.2/subprocess.py | 3 ++- test-data/stdlib-samples/3.2/tempfile.py | 11 +++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/test-data/stdlib-samples/3.2/subprocess.py b/test-data/stdlib-samples/3.2/subprocess.py index 70f5c8d700b64..32c2ee729791c 100644 --- a/test-data/stdlib-samples/3.2/subprocess.py +++ b/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: diff --git a/test-data/stdlib-samples/3.2/tempfile.py b/test-data/stdlib-samples/3.2/tempfile.py index f8deef01e4630..cfc657c5e1b13 100644 --- a/test-data/stdlib-samples/3.2/tempfile.py +++ b/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 From 2ccb0323ea8819f3e711f23975ccd00283256e3c Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Tue, 8 Oct 2019 14:13:59 +0100 Subject: [PATCH 7/8] Fix lint --- mypy/messages.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mypy/messages.py b/mypy/messages.py index 8eff1e6ebcbd0..eceec190020fa 100644 --- a/mypy/messages.py +++ b/mypy/messages.py @@ -1107,7 +1107,7 @@ def incorrectly_returning_any(self, typ: Type, context: Context) -> None: 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) + 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) From f0507a206145348c41187551fbafa1d28128a0c9 Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Tue, 8 Oct 2019 15:32:03 +0100 Subject: [PATCH 8/8] Document the error code --- docs/source/error_code_list.rst | 57 +++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/docs/source/error_code_list.rst b/docs/source/error_code_list.rst index 133f315e19d54..376fe5e1a1a7e 100644 --- a/docs/source/error_code_list.rst +++ b/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] -----------------------------