From b56fafb760d1b2e40911bb8667b188f441ea4232 Mon Sep 17 00:00:00 2001 From: Jared Hance Date: Wed, 2 Mar 2022 10:28:34 -0800 Subject: [PATCH 1/5] Error on unused awaitable expressions Generates an error when an expression has a type which has a defined __await__ in its MRO but is not used. This includes all builtin awaitable objects (e.g. futures, coroutines, and tasks) but also anything a user might define which is also awaitable. A hint is attached to suggest awaiting. This can be extended in the future to other types of values that may lead to a resource leak if needed, or even exposed as a plugin. We test simple and complex cases (coroutines and user defined classes). We also test that __getattr__ does not create false positives for awaitables. Some tests require fixes, either because they were deliberately not awaiting an awaitable to verify some other logic in mypy, or because reveal_type returns the object, so it was generating an error we would rather simply silence. --- mypy/checker.py | 18 ++++++++++++++- mypy/message_registry.py | 1 + test-data/unit/check-async-await.test | 27 +++++++++++++++++++++- test-data/unit/check-class-namedtuple.test | 2 +- test-data/unit/check-flags.test | 3 ++- 5 files changed, 47 insertions(+), 4 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index 851f23185f4f..35ae9282414f 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -3432,8 +3432,24 @@ def try_infer_partial_type_from_indexed_assignment( [key_type, value_type]) del partial_types[var] + def type_requires_usage(self, typ: Type) -> Optional[str]: + """Some types require usage in all cases. The classic example is + an unused coroutine. + + In the case that it does require usage, returns a note to attach + to the error message. + """ + if isinstance(typ, Instance): + if typ.type.get("__await__") is not None: + return "Are you missing an await?" + return None + def visit_expression_stmt(self, s: ExpressionStmt) -> None: - self.expr_checker.accept(s.expr, allow_none_return=True, always_allow_any=True) + expr_type = self.expr_checker.accept(s.expr, allow_none_return=True, always_allow_any=True) + error_note = self.type_requires_usage(expr_type) + if error_note: + self.fail(message_registry.TYPE_MUST_BE_USED.format(format_type(expr_type)), s) + self.note(error_note, s) def visit_return_stmt(self, s: ReturnStmt) -> None: """Type check a return statement.""" diff --git a/mypy/message_registry.py b/mypy/message_registry.py index 1477cc4da575..68e739b24358 100644 --- a/mypy/message_registry.py +++ b/mypy/message_registry.py @@ -141,6 +141,7 @@ def format(self, *args: object, **kwargs: object) -> "ErrorMessage": PYTHON2_PRINT_FILE_TYPE: Final = ( 'Argument "file" to "print" has incompatible type "{}"; expected "{}"' ) +TYPE_MUST_BE_USED: Final = 'Value of type {} must be used' # Generic GENERIC_INSTANCE_VAR_CLASS_ACCESS: Final = ( diff --git a/test-data/unit/check-async-await.test b/test-data/unit/check-async-await.test index f4bf7c7cfa94..6923ac9a4b55 100644 --- a/test-data/unit/check-async-await.test +++ b/test-data/unit/check-async-await.test @@ -12,7 +12,7 @@ async def f() -> int: async def f() -> int: return 0 -reveal_type(f()) # N: Revealed type is "typing.Coroutine[Any, Any, builtins.int]" +_ = reveal_type(f()) # N: Revealed type is "typing.Coroutine[Any, Any, builtins.int]" [builtins fixtures/async_await.pyi] [typing fixtures/typing-async.pyi] @@ -799,3 +799,28 @@ async def precise2(futures: Iterable[Awaitable[int]]) -> None: [builtins fixtures/async_await.pyi] [typing fixtures/typing-async.pyi] + +[case testUnusedAwaitable] +from typing import Iterable + +async def foo() -> None: + pass + +class A: + def __await__(self) -> Iterable[int]: + yield 5 + +# Things with __getattr__ should not simply be considered awaitable. +class B: + def __getattr__(self, attr) -> object: + return 0 + +def bar() -> None: + foo() # E: Value of type "Coroutine[Any, Any, None]" must be used \ + # N: Are you missing an await? + A() # E: Value of type "A" must be used \ + # N: Are you missing an await? + B() + +[builtins fixtures/async_await.pyi] +[typing fixtures/typing-async.pyi] diff --git a/test-data/unit/check-class-namedtuple.test b/test-data/unit/check-class-namedtuple.test index cf3af49ab3a4..4a1a84ca0301 100644 --- a/test-data/unit/check-class-namedtuple.test +++ b/test-data/unit/check-class-namedtuple.test @@ -538,7 +538,7 @@ class XRepr(NamedTuple): return 0 reveal_type(XMeth(1).double()) # N: Revealed type is "builtins.int" -reveal_type(XMeth(1).asyncdouble()) # N: Revealed type is "typing.Coroutine[Any, Any, builtins.int]" +_ = reveal_type(XMeth(1).asyncdouble()) # N: Revealed type is "typing.Coroutine[Any, Any, builtins.int]" reveal_type(XMeth(42).x) # N: Revealed type is "builtins.int" reveal_type(XRepr(42).__str__()) # N: Revealed type is "builtins.str" reveal_type(XRepr(1, 2).__sub__(XRepr(3))) # N: Revealed type is "builtins.int" diff --git a/test-data/unit/check-flags.test b/test-data/unit/check-flags.test index 8f72a82b0760..8ee0adce3c8e 100644 --- a/test-data/unit/check-flags.test +++ b/test-data/unit/check-flags.test @@ -405,7 +405,8 @@ async def g() -> NoReturn: await f() async def h() -> NoReturn: # E: Implicit return in function which does not return - f() + # Purposely not evaluating coroutine + _ = f() [builtins fixtures/dict.pyi] [typing fixtures/typing-async.pyi] From e9ebeb89e45ff2b89ec5ba7877fef79efaa724b8 Mon Sep 17 00:00:00 2001 From: Jared Hance Date: Thu, 3 Mar 2022 10:43:09 -0800 Subject: [PATCH 2/5] Fix failing tests --- test-data/unit/pythoneval-asyncio.test | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test-data/unit/pythoneval-asyncio.test b/test-data/unit/pythoneval-asyncio.test index b3400fe6010e..a76ece37da40 100644 --- a/test-data/unit/pythoneval-asyncio.test +++ b/test-data/unit/pythoneval-asyncio.test @@ -78,7 +78,7 @@ def slow_operation(future: 'Future[str]') -> 'Generator[Any, None, None]': loop = asyncio.get_event_loop() future = asyncio.Future() # type: Future[str] -asyncio.Task(slow_operation(future)) +_ = asyncio.Task(slow_operation(future)) loop.run_until_complete(future) print(future.result()) loop.close() @@ -102,7 +102,7 @@ def got_result(future: 'Future[str]') -> None: loop = asyncio.get_event_loop() # type: AbstractEventLoop future = asyncio.Future() # type: Future[str] -asyncio.Task(slow_operation(future)) # Here create a task with the function. (The Task need a Future[T] as first argument) +_ = asyncio.Task(slow_operation(future)) # Here create a task with the function. (The Task need a Future[T] as first argument) future.add_done_callback(got_result) # and assign the callback to the future try: loop.run_forever() @@ -314,7 +314,7 @@ def slow_operation(future: 'Future[str]') -> 'Generator[Any, None, None]': loop = asyncio.get_event_loop() future = asyncio.Future() # type: Future[str] -asyncio.Task(slow_operation(future)) +_ = asyncio.Task(slow_operation(future)) loop.run_until_complete(future) print(future.result()) loop.close() @@ -334,7 +334,7 @@ def slow_operation(future: 'Future[int]') -> 'Generator[Any, None, None]': loop = asyncio.get_event_loop() future = asyncio.Future() # type: Future[str] -asyncio.Task(slow_operation(future)) # Error +_ = asyncio.Task(slow_operation(future)) # Error loop.run_until_complete(future) print(future.result()) loop.close() @@ -353,7 +353,7 @@ def slow_operation(future: 'Future[int]') -> 'Generator[Any, None, None]': loop = asyncio.get_event_loop() future = asyncio.Future() # type: Future[str] -asyncio.Task(slow_operation(future)) # Error +_ = asyncio.Task(slow_operation(future)) # Error loop.run_until_complete(future) print(future.result()) loop.close() @@ -378,7 +378,7 @@ def got_result(future: 'Future[int]') -> None: loop = asyncio.get_event_loop() # type: AbstractEventLoop future = asyncio.Future() # type: Future[str] -asyncio.Task(slow_operation(future)) +_ = asyncio.Task(slow_operation(future)) future.add_done_callback(got_result) # Error try: From 04bf027c46ed215792911e2cb872f7b258354ba3 Mon Sep 17 00:00:00 2001 From: Jared Hance Date: Wed, 9 Mar 2022 10:05:54 -0800 Subject: [PATCH 3/5] Split into two error codes by default --- mypy/checker.py | 24 +++++++++++++++--------- mypy/errorcodes.py | 9 +++++++++ test-data/unit/check-async-await.test | 7 ++++--- test-data/unit/pythoneval-asyncio.test | 12 ++++++------ 4 files changed, 34 insertions(+), 18 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index 35ae9282414f..4888001550ad 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -85,7 +85,7 @@ from mypy.scope import Scope from mypy import state, errorcodes as codes from mypy.traverser import has_return_statement, all_return_statements -from mypy.errorcodes import ErrorCode +from mypy.errorcodes import ErrorCode, UNUSED_AWAITABLE, UNUSED_COROUTINE from mypy.util import is_typeshed_file, is_dunder, is_sunder T = TypeVar('T') @@ -3432,24 +3432,30 @@ def try_infer_partial_type_from_indexed_assignment( [key_type, value_type]) del partial_types[var] - def type_requires_usage(self, typ: Type) -> Optional[str]: + def type_requires_usage(self, typ: Type) -> Optional[Tuple[str, ErrorCode]]: """Some types require usage in all cases. The classic example is an unused coroutine. In the case that it does require usage, returns a note to attach to the error message. """ - if isinstance(typ, Instance): - if typ.type.get("__await__") is not None: - return "Are you missing an await?" + proper_type = get_proper_type(typ) + if isinstance(proper_type, Instance): + # We use different error codes for generic awaitable vs coroutine. + # Coroutines are on by default, whereas generic awaitables are not. + if proper_type.type.fullname == "typing.Coroutine": + return ("Are you missing an await?", UNUSED_COROUTINE) + if proper_type.type.get("__await__") is not None: + return ("Are you missing an await?", UNUSED_AWAITABLE) return None def visit_expression_stmt(self, s: ExpressionStmt) -> None: expr_type = self.expr_checker.accept(s.expr, allow_none_return=True, always_allow_any=True) - error_note = self.type_requires_usage(expr_type) - if error_note: - self.fail(message_registry.TYPE_MUST_BE_USED.format(format_type(expr_type)), s) - self.note(error_note, s) + error_note_and_code = self.type_requires_usage(expr_type) + if error_note_and_code: + error_note, code = error_note_and_code + self.fail(message_registry.TYPE_MUST_BE_USED.format(format_type(expr_type)), s, code=code) + self.note(error_note, s, code=code) def visit_return_stmt(self, s: ReturnStmt) -> None: """Type check a return statement.""" diff --git a/mypy/errorcodes.py b/mypy/errorcodes.py index 4407df47d596..81d8df240a2f 100644 --- a/mypy/errorcodes.py +++ b/mypy/errorcodes.py @@ -97,6 +97,9 @@ def __str__(self) -> str: LITERAL_REQ: Final = ErrorCode( "literal-required", "Check that value is a literal", 'General' ) +UNUSED_COROUTINE: Final = ErrorCode( + "unused-coroutine", "Ensure that all coroutines are used", "Genera" +) # These error codes aren't enabled by default. NO_UNTYPED_DEF: Final[ErrorCode] = ErrorCode( @@ -147,6 +150,12 @@ def __str__(self) -> str: "General", default_enabled=False, ) +UNUSED_AWAITABLE: Final = ErrorCode( + "unused-awaitable", + "Ensure that all coroutines are used", + "General", + default_enabled=False, +) # Syntax errors are often blocking. diff --git a/test-data/unit/check-async-await.test b/test-data/unit/check-async-await.test index 6923ac9a4b55..382d788ee904 100644 --- a/test-data/unit/check-async-await.test +++ b/test-data/unit/check-async-await.test @@ -801,6 +801,7 @@ async def precise2(futures: Iterable[Awaitable[int]]) -> None: [typing fixtures/typing-async.pyi] [case testUnusedAwaitable] +# flags: --show-error-codes --enable-error-code unused-awaitable from typing import Iterable async def foo() -> None: @@ -816,10 +817,10 @@ class B: return 0 def bar() -> None: - foo() # E: Value of type "Coroutine[Any, Any, None]" must be used \ - # N: Are you missing an await? - A() # E: Value of type "A" must be used \ + A() # E: Value of type "A" must be used [unused-awaitable] \ # N: Are you missing an await? + foo() # E: Value of type "Coroutine[Any, Any, None]" must be used [unused-coroutine] \ + # N: Are you missing an await? B() [builtins fixtures/async_await.pyi] diff --git a/test-data/unit/pythoneval-asyncio.test b/test-data/unit/pythoneval-asyncio.test index a76ece37da40..b3400fe6010e 100644 --- a/test-data/unit/pythoneval-asyncio.test +++ b/test-data/unit/pythoneval-asyncio.test @@ -78,7 +78,7 @@ def slow_operation(future: 'Future[str]') -> 'Generator[Any, None, None]': loop = asyncio.get_event_loop() future = asyncio.Future() # type: Future[str] -_ = asyncio.Task(slow_operation(future)) +asyncio.Task(slow_operation(future)) loop.run_until_complete(future) print(future.result()) loop.close() @@ -102,7 +102,7 @@ def got_result(future: 'Future[str]') -> None: loop = asyncio.get_event_loop() # type: AbstractEventLoop future = asyncio.Future() # type: Future[str] -_ = asyncio.Task(slow_operation(future)) # Here create a task with the function. (The Task need a Future[T] as first argument) +asyncio.Task(slow_operation(future)) # Here create a task with the function. (The Task need a Future[T] as first argument) future.add_done_callback(got_result) # and assign the callback to the future try: loop.run_forever() @@ -314,7 +314,7 @@ def slow_operation(future: 'Future[str]') -> 'Generator[Any, None, None]': loop = asyncio.get_event_loop() future = asyncio.Future() # type: Future[str] -_ = asyncio.Task(slow_operation(future)) +asyncio.Task(slow_operation(future)) loop.run_until_complete(future) print(future.result()) loop.close() @@ -334,7 +334,7 @@ def slow_operation(future: 'Future[int]') -> 'Generator[Any, None, None]': loop = asyncio.get_event_loop() future = asyncio.Future() # type: Future[str] -_ = asyncio.Task(slow_operation(future)) # Error +asyncio.Task(slow_operation(future)) # Error loop.run_until_complete(future) print(future.result()) loop.close() @@ -353,7 +353,7 @@ def slow_operation(future: 'Future[int]') -> 'Generator[Any, None, None]': loop = asyncio.get_event_loop() future = asyncio.Future() # type: Future[str] -_ = asyncio.Task(slow_operation(future)) # Error +asyncio.Task(slow_operation(future)) # Error loop.run_until_complete(future) print(future.result()) loop.close() @@ -378,7 +378,7 @@ def got_result(future: 'Future[int]') -> None: loop = asyncio.get_event_loop() # type: AbstractEventLoop future = asyncio.Future() # type: Future[str] -_ = asyncio.Task(slow_operation(future)) +asyncio.Task(slow_operation(future)) future.add_done_callback(got_result) # Error try: From 4c91521e4b60f1414912277ef940dad5b4efa5f0 Mon Sep 17 00:00:00 2001 From: Jared Hance Date: Wed, 9 Mar 2022 10:07:24 -0800 Subject: [PATCH 4/5] Flake8 --- mypy/checker.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/mypy/checker.py b/mypy/checker.py index 4888001550ad..ec229abd3801 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -3454,7 +3454,9 @@ def visit_expression_stmt(self, s: ExpressionStmt) -> None: error_note_and_code = self.type_requires_usage(expr_type) if error_note_and_code: error_note, code = error_note_and_code - self.fail(message_registry.TYPE_MUST_BE_USED.format(format_type(expr_type)), s, code=code) + self.fail( + message_registry.TYPE_MUST_BE_USED.format(format_type(expr_type)), s, code=code + ) self.note(error_note, s, code=code) def visit_return_stmt(self, s: ReturnStmt) -> None: From d2ad5ec1bb679563f004cb04ba9062c7aff7efee Mon Sep 17 00:00:00 2001 From: Jared Hance Date: Thu, 10 Mar 2022 20:19:55 -0800 Subject: [PATCH 5/5] codes --- mypy/errorcodes.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mypy/errorcodes.py b/mypy/errorcodes.py index 81d8df240a2f..31f1a2fb73ea 100644 --- a/mypy/errorcodes.py +++ b/mypy/errorcodes.py @@ -98,7 +98,7 @@ def __str__(self) -> str: "literal-required", "Check that value is a literal", 'General' ) UNUSED_COROUTINE: Final = ErrorCode( - "unused-coroutine", "Ensure that all coroutines are used", "Genera" + "unused-coroutine", "Ensure that all coroutines are used", "General" ) # These error codes aren't enabled by default. @@ -152,7 +152,7 @@ def __str__(self) -> str: ) UNUSED_AWAITABLE: Final = ErrorCode( "unused-awaitable", - "Ensure that all coroutines are used", + "Ensure that all awaitable values are used", "General", default_enabled=False, )