[ruff] Implement missing await for coroutine (RUF028)
#9911
Conversation
|
CodSpeed Performance ReportMerging #9911 will not alter performanceComparing Summary
|
|
Thanks for contributing! It looks like the ecosystem checks are showing some false positives e.g. https://github.com/apache/airflow/blob/48bfb1a970f5b47ba1b385ad809b8324923ddf3e/tests/providers/google/cloud/hooks/test_dataflow.py#L1956 — could you look into those? |
Thanks! Good catch. |
|
This rule fits into ruff as a suspicious rule. I think we should rename the rule to |
| pass | ||
| async def coro(): | ||
| return another_coro() | ||
| await coro() # OK |
There was a problem hiding this comment.
is this ok? another_coro is never awaited. if you await coro() you will get an unevaluated coro.
this works tho:
async def test_coroutine_with_sync_return():
async def another_coro():
pass
def coro():
return another_coro()
await coro() # OK
|
Hi, any update on when this will be released? |
There was a problem hiding this comment.
Thank you for implementing this rule!
Can either @zanieb or @AlexWaygood look at this? It's good to go from my side but I've mainly looked at the code part of the rule as I'm not an async expert :)
| /// Generate a [`Fix`] to add `await` for coroutine. | ||
| /// | ||
| /// For example: | ||
| /// - Given `asyncio.sleep(1)`, generate `await asyncio.sleep(1)`. | ||
| fn generate_fix(call: &ExprCall) -> Expr { |
There was a problem hiding this comment.
The function docs doesn't match what it actually returns. I think we should either return the Fix as mentioned in the docs or update the name to something like create_await_expr. I would recommend to go with the former.
| return; | ||
| } | ||
|
|
||
| let mut diagnostic = Diagnostic::new(MissingAwaitForCoroutine {}, call.range); |
There was a problem hiding this comment.
The curly braces aren't required for empty structs
| let mut diagnostic = Diagnostic::new(MissingAwaitForCoroutine {}, call.range); | |
| let mut diagnostic = Diagnostic::new(MissingAwaitForCoroutine, call.range); |
| // Try to detect possible scenarios where await is missing and ignore other cases | ||
| // For example, if the call is not a direct child of an statement expression or assignment statement | ||
| // then it's not reliable to determine if await is missing. | ||
| // User might return coroutine object from a function or pass it as an argument | ||
| if !possibly_missing_await(call, checker.semantic()) { |
There was a problem hiding this comment.
I think it might be useful to move the comment as part of possibly_missing_await's documentation.
| diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement( | ||
| checker.generator().expr(&generate_fix(call)), | ||
| call.range, | ||
| ))); |
There was a problem hiding this comment.
nit: we could possibly just create an Insertion edit instead at the start of the call expression.
| diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement( | |
| checker.generator().expr(&generate_fix(call)), | |
| call.range, | |
| ))); | |
| diagnostic.set_fix(Fix::unsafe_edit(Edit::insertion( | |
| "await ".to_string(), | |
| call.start(), | |
| ))); |
which is basically what the generator does as well ;)
ruff/crates/ruff_python_codegen/src/generator.rs
Lines 980 to 985 in f76a3e8
| // For example, if the call is not a direct child of an statement expression or assignment statement | ||
| // then it's not reliable to determine if await is missing. | ||
| // User might return coroutine object from a function or pass it as an argument |
There was a problem hiding this comment.
nit: Do you think it would be helpful for the user to know this limitation? If so, we could possibly provide a "Limitations" section in the rule documentation. For example, refer https://docs.astral.sh/ruff/rules/implicit-optional/#limitations.
RUF028) Implement missing await for coroutineruff] Implement missing await for coroutine (RUF028)
|
Hi again, when can we expect this to be released? |
Summary
Detect never-awaited coroutines
This rule only applies to simple scenarios. We need to be quite conservative on how to handle coroutines. Because there are many situations in which the programmer does not want to evaluate/await a coroutine but instead use the actual coroutine object.
Closes #9833
Test Plan
cargo test