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

Error on unused awaitable expressions #12279

Merged
merged 5 commits into from
Mar 11, 2022
Merged

Conversation

jhance
Copy link
Collaborator

@jhance jhance commented Mar 2, 2022

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.

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.
@github-actions

This comment has been minimized.

@JelleZijlstra
Copy link
Member

This one is a false positive: https://github.com/aiortc/aiortc/blob/c258ef4167eb04914646db03c6044ca05428893a/src/aiortc/rtcsctptransport.py#L1393. There's no need to await asyncio.ensure_future. And in any case, you can't add an await in a non-async function.

@jhance
Copy link
Collaborator Author

jhance commented Mar 2, 2022

Failure to use a future likely indicates a resource leak bug because you won't cancel in the event of failure, so I wouldn't classify it as a false positive.

I think its reasonable that if you don't want to use the value you do _ =.

@jhance
Copy link
Collaborator Author

jhance commented Mar 2, 2022

Also, docs explicitly say not to throw away the result of ensure_future or create_task: https://docs.python.org/3/library/asyncio-task.html#asyncio.create_task

@github-actions

This comment has been minimized.

@JukkaL
Copy link
Collaborator

JukkaL commented Mar 7, 2022

At least homeassistant seems to have some false positives. It has a async_create_task method that returns a Task, but it also records the task object so that actually won't be leaked, so the return value can be ignored.

I can suggest a few things we could do about these:

  1. Only complain about unused Task (or generally non-coroutine) values from certain known asyncio methods whose return values should not be ignored.
  2. Use different error codes for ignored coroutines and other awaitables. Only complain about coroutines by default, and the general awaitable errors would have to be explicitly enabled, since they can cause false positives.

In any case, it would be good to use a dedicated error code for these errors.

There's also a self-check error:

mypy/checker.py:3442: error: Never apply isinstance() to unexpanded types; use
mypy.types.get_proper_type() first  [misc]
            if isinstance(typ, Instance):
               ^

@jhance
Copy link
Collaborator Author

jhance commented Mar 9, 2022

Feature is split into two error codes so that the more controversial/harder one (unused task/future) is not on by default but unused coroutine, which everyone should want, is on by default.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now! Left two small comments/suggestions. Feel free to merge after you've addressed them.

@@ -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"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"unused-coroutine", "Ensure that all coroutines are used", "Genera"
"unused-coroutine", "Ensure that all coroutines are used", "General"

@@ -147,6 +150,12 @@ def __str__(self) -> str:
"General",
default_enabled=False,
)
UNUSED_AWAITABLE: Final = ErrorCode(
"unused-awaitable",
"Ensure that all coroutines are used",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the description say something like "all awaitable values"?

@jhance jhance merged commit 536bac0 into python:master Mar 11, 2022
@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

edgedb (https://github.com/edgedb/edgedb)
+ edb/tools/gen_test_dumps.py:149:9: error: Value of type "Coroutine[Any, Any, None]" must be used
+ edb/tools/gen_test_dumps.py:149:9: note: Are you missing an await?

dragonchain (https://github.com/dragonchain/dragonchain)
+ dragonchain/broadcast_processor/broadcast_processor.py:190:17: error: Value of type "Coroutine[Any, Any, int]" must be used
+ dragonchain/broadcast_processor/broadcast_processor.py:190:17: note: Are you missing an await?

pankajkoti added a commit to astronomer/astronomer-providers that referenced this pull request Apr 29, 2022
We're facing an issue where mypy is compalining that we're missing
an await on `asyncio.sleep(polling_interval)` in
astronomer/providers/apache/hive/hooks/hive.py:46
This is potentially because of the new release of mypy==0.950
including PR python/mypy#12279
We don't face this issue with mypy==0.942 and hence, freezing it.
pankajkoti added a commit to astronomer/astronomer-providers that referenced this pull request Apr 29, 2022
We're facing an issue where mypy is compalining that we're missing
an await on `asyncio.sleep(polling_interval)` in
astronomer/providers/apache/hive/hooks/hive.py:46
This is potentially because of the new release of mypy==0.950
including PR python/mypy#12279
We don't face this issue with mypy==0.942 and hence, freezing it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants