-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[ty] Add a diagnostic for an unused awaitable #23650
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,151 @@ | ||
| # Unused awaitable | ||
|
|
||
| ## Basic coroutine not awaited | ||
|
|
||
| Calling an `async def` function produces a coroutine that must be awaited. | ||
|
|
||
| ```py | ||
| async def fetch() -> int: | ||
| return 42 | ||
|
|
||
| async def main(): | ||
| fetch() # error: [unused-awaitable] | ||
| ``` | ||
|
|
||
| ## Awaited coroutine is fine | ||
|
|
||
| ```py | ||
| async def fetch() -> int: | ||
| return 42 | ||
|
|
||
| async def main(): | ||
| await fetch() | ||
| ``` | ||
|
|
||
| ## Assigned coroutine is fine | ||
|
|
||
| ```py | ||
| async def fetch() -> int: | ||
| return 42 | ||
|
|
||
| async def main(): | ||
| # TODO: ty should eventually warn about unused coroutines assigned to variables | ||
| coro = fetch() | ||
| ``` | ||
|
|
||
| ## Coroutine passed to a function | ||
|
|
||
| When a coroutine is passed as an argument rather than used as an expression statement, no diagnostic | ||
| should be emitted. | ||
|
|
||
| ```py | ||
| async def fetch() -> int: | ||
| return 42 | ||
|
|
||
| async def main(): | ||
| print(fetch()) | ||
| ``` | ||
|
|
||
| ## Top-level coroutine call | ||
|
|
||
| The lint fires even outside of `async def`, since the coroutine is still discarded. | ||
|
|
||
| ```py | ||
| async def fetch() -> int: | ||
| return 42 | ||
|
|
||
| fetch() # error: [unused-awaitable] | ||
| ``` | ||
|
|
||
| ## Union of awaitables | ||
|
|
||
| When every element of a union is awaitable, the lint should fire. | ||
|
|
||
| ```py | ||
| from types import CoroutineType | ||
| from typing import Any | ||
|
|
||
| def get_coroutine() -> CoroutineType[Any, Any, int] | CoroutineType[Any, Any, str]: | ||
| raise NotImplementedError | ||
|
|
||
| async def main(): | ||
| get_coroutine() # error: [unused-awaitable] | ||
| ``` | ||
|
|
||
| ## Union with non-awaitable | ||
|
|
||
| When a union contains a non-awaitable element, the lint should not fire. | ||
|
|
||
| ```py | ||
| from types import CoroutineType | ||
| from typing import Any | ||
|
|
||
| def get_maybe_coroutine() -> CoroutineType[Any, Any, int] | int: | ||
| raise NotImplementedError | ||
|
|
||
| async def main(): | ||
| get_maybe_coroutine() | ||
| ``` | ||
|
|
||
| ## Intersection with awaitable | ||
|
|
||
| When an intersection type contains an awaitable element, the lint should fire. | ||
|
|
||
| ```py | ||
| from collections.abc import Coroutine | ||
| from types import CoroutineType | ||
| from ty_extensions import Intersection | ||
|
|
||
| class Foo: ... | ||
| class Bar: ... | ||
|
|
||
| def get_coroutine() -> Intersection[Coroutine[Foo, Foo, Foo], CoroutineType[Bar, Bar, Bar]]: | ||
| raise NotImplementedError | ||
|
|
||
| async def main(): | ||
| get_coroutine() # error: [unused-awaitable] | ||
| ``` | ||
|
|
||
| ## `reveal_type` and `assert_type` are not flagged | ||
|
|
||
| Calls to `reveal_type` and `assert_type` should not trigger this lint, even when their argument is | ||
| an awaitable. | ||
|
|
||
| ```py | ||
| from typing_extensions import assert_type | ||
| from types import CoroutineType | ||
| from typing import Any | ||
|
|
||
| async def fetch() -> int: | ||
| return 42 | ||
|
|
||
| async def main(): | ||
| reveal_type(fetch()) # revealed: CoroutineType[Any, Any, int] | ||
| assert_type(fetch(), CoroutineType[Any, Any, int]) | ||
| ``` | ||
|
|
||
| ## Non-awaitable expression statement | ||
|
|
||
| Regular non-awaitable expression statements should not trigger this lint. | ||
|
|
||
| ```py | ||
| def compute() -> int: | ||
| return 42 | ||
|
|
||
| def main(): | ||
| compute() | ||
| ``` | ||
|
|
||
| ## Dynamic type | ||
|
|
||
| `Any` and `Unknown` types should not trigger the lint. | ||
|
|
||
| ```py | ||
| from typing import Any | ||
|
|
||
| def get_any() -> Any: | ||
| return None | ||
|
|
||
| async def main(): | ||
| get_any() | ||
| ``` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -995,6 +995,30 @@ impl<'db> Type<'db> { | |
| self.is_dynamic() && !self.is_divergent() | ||
| } | ||
|
|
||
| /// Returns `true` if this type is an awaitable that should be awaited before being discarded. | ||
| /// | ||
| /// Currently checks for instances of `types.CoroutineType` (returned by `async def` calls). | ||
| /// Unions are considered awaitable only if every element is awaitable. | ||
| /// Intersections are considered awaitable if any positive element is awaitable. | ||
| pub(crate) fn is_awaitable(self, db: &'db dyn Db) -> bool { | ||
| match self { | ||
| Type::NominalInstance(instance) => { | ||
| matches!(instance.known_class(db), Some(KnownClass::CoroutineType)) | ||
| } | ||
| Type::Union(union) => { | ||
| let elements = union.elements(db); | ||
| // Guard against empty unions (`Never`), since `all()` on an empty | ||
| // iterator returns `true`. | ||
| !elements.is_empty() && elements.iter().all(|ty| ty.is_awaitable(db)) | ||
| } | ||
| Type::Intersection(intersection) => intersection | ||
| .positive(db) | ||
| .iter() | ||
| .any(|ty| ty.is_awaitable(db)), | ||
|
Comment on lines
+1014
to
+1017
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably have a test case for this branch. (Formatting diffs with ``` in them is hard...) |
||
| _ => false, | ||
| } | ||
| } | ||
|
|
||
| /// Is a value of this type only usable in typing contexts? | ||
| pub fn is_type_check_only(&self, db: &'db dyn Db) -> bool { | ||
| match self { | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, did you consider checking for "assignability to
Awaitable"? My instinct is that that's too expensive to evaluate on every single expression statement, but I'm not sure.