[flake8-async] Fix false positives with multiple async with items (ASYNC100) (#12643)

## Summary

Please see
https://github.com/astral-sh/ruff/pull/12605#discussion_r1699957443 for
a description of the issue.

They way I fixed it is to get the *last* timeout item in the `with`, and
if it's an `async with` and there are items after it, then don't trigger
the lint.

## Test Plan

Updated the fixture with some more cases.
This commit is contained in:
Ran Benita 2024-08-03 00:25:13 +03:00 committed by GitHub
parent 1c311e4fdb
commit fbfe2cb2f5
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 126 additions and 78 deletions

View file

@ -1,51 +1,63 @@
import anyio import anyio
import asyncio import asyncio
import trio import trio
from contextlib import nullcontext
async def func(): async def func():
async with trio.fail_after(): with trio.fail_after():
... ...
async def func(): async def func():
async with trio.fail_at(): with trio.fail_at():
await ... await ...
async def func(): async def func():
async with trio.move_on_after(): with trio.move_on_after():
... ...
async def func(): async def func():
async with trio.move_at(): with trio.move_at():
await ... await ...
async def func(): async def func():
async with trio.move_at(): with trio.move_at():
async with trio.open_nursery(): async with trio.open_nursery():
... ...
async def func(): async def func():
async with anyio.move_on_after(delay=0.2): with trio.move_at():
async for x in ...:
...
async def func():
with anyio.move_on_after(delay=0.2):
... ...
async def func(): async def func():
async with anyio.fail_after(): with anyio.fail_after():
... ...
async def func(): async def func():
async with anyio.CancelScope(): with anyio.CancelScope():
... ...
async def func(): async def func():
async with anyio.CancelScope(): with anyio.CancelScope(), nullcontext():
...
async def func():
with nullcontext(), anyio.CancelScope():
... ...
@ -62,3 +74,18 @@ async def func():
async def func(): async def func():
async with asyncio.timeout(delay=0.2), asyncio.TaskGroup(): async with asyncio.timeout(delay=0.2), asyncio.TaskGroup():
... ...
async def func():
async with asyncio.timeout(delay=0.2), asyncio.TaskGroup(), asyncio.timeout(delay=0.2):
...
async def func():
async with asyncio.timeout(delay=0.2), asyncio.TaskGroup(), asyncio.timeout(delay=0.2), asyncio.TaskGroup():
...
async def func():
async with asyncio.timeout(delay=0.2), asyncio.timeout(delay=0.2):
...

View file

@ -13,9 +13,9 @@ use crate::settings::types::PreviewMode;
/// ///
/// ## Why is this bad? /// ## Why is this bad?
/// Some asynchronous context managers, such as `asyncio.timeout` and /// Some asynchronous context managers, such as `asyncio.timeout` and
/// `trio.move_on_after`, have no effect unless they contain an `await` /// `trio.move_on_after`, have no effect unless they contain a checkpoint.
/// statement. The use of such context managers without an `await` statement is /// The use of such context managers without an `await`, `async with` or
/// likely a mistake. /// `async for` statement is likely a mistake.
/// ///
/// ## Example /// ## Example
/// ```python /// ```python
@ -55,17 +55,29 @@ pub(crate) fn cancel_scope_no_checkpoint(
with_stmt: &StmtWith, with_stmt: &StmtWith,
with_items: &[WithItem], with_items: &[WithItem],
) { ) {
let Some(method_name) = with_items.iter().find_map(|item| { let Some((with_item_pos, method_name)) = with_items
let call = item.context_expr.as_call_expr()?; .iter()
let qualified_name = checker .enumerate()
.semantic() .filter_map(|(pos, item)| {
.resolve_qualified_name(call.func.as_ref())?; let call = item.context_expr.as_call_expr()?;
MethodName::try_from(&qualified_name) let qualified_name = checker
}) else { .semantic()
.resolve_qualified_name(call.func.as_ref())?;
let method_name = MethodName::try_from(&qualified_name)?;
if method_name.is_timeout_context() {
Some((pos, method_name))
} else {
None
}
})
.last()
else {
return; return;
}; };
if !method_name.is_timeout_context() { // If this is an `async with` and the timeout has items after it, then the
// further items are checkpoints.
if with_stmt.is_async && with_item_pos < with_items.len() - 1 {
return; return;
} }
@ -76,24 +88,6 @@ pub(crate) fn cancel_scope_no_checkpoint(
return; return;
} }
// If there's an `asyncio.TaskGroup()` context manager alongside the timeout, it's fine, as in:
// ```python
// async with asyncio.timeout(2.0), asyncio.TaskGroup():
// ...
// ```
if with_items.iter().any(|item| {
item.context_expr.as_call_expr().is_some_and(|call| {
checker
.semantic()
.resolve_qualified_name(call.func.as_ref())
.is_some_and(|qualified_name| {
matches!(qualified_name.segments(), ["asyncio", "TaskGroup"])
})
})
}) {
return;
}
if matches!(checker.settings.preview, PreviewMode::Disabled) { if matches!(checker.settings.preview, PreviewMode::Disabled) {
if matches!(method_name.module(), AsyncModule::Trio) { if matches!(method_name.module(), AsyncModule::Trio) {
checker.diagnostics.push(Diagnostic::new( checker.diagnostics.push(Diagnostic::new(

View file

@ -1,20 +1,20 @@
--- ---
source: crates/ruff_linter/src/rules/flake8_async/mod.rs source: crates/ruff_linter/src/rules/flake8_async/mod.rs
--- ---
ASYNC100.py:7:5: ASYNC100 A `with trio.fail_after(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint. ASYNC100.py:8:5: ASYNC100 A `with trio.fail_after(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint.
| |
6 | async def func(): 7 | async def func():
7 | async with trio.fail_after(): 8 | with trio.fail_after():
| _____^ | _____^
8 | | ... 9 | | ...
| |___________^ ASYNC100 | |___________^ ASYNC100
| |
ASYNC100.py:17:5: ASYNC100 A `with trio.move_on_after(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint. ASYNC100.py:18:5: ASYNC100 A `with trio.move_on_after(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint.
| |
16 | async def func(): 17 | async def func():
17 | async with trio.move_on_after(): 18 | with trio.move_on_after():
| _____^ | _____^
18 | | ... 19 | | ...
| |___________^ ASYNC100 | |___________^ ASYNC100
| |

View file

@ -1,74 +1,101 @@
--- ---
source: crates/ruff_linter/src/rules/flake8_async/mod.rs source: crates/ruff_linter/src/rules/flake8_async/mod.rs
--- ---
ASYNC100.py:7:5: ASYNC100 A `with trio.fail_after(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint. ASYNC100.py:8:5: ASYNC100 A `with trio.fail_after(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint.
| |
6 | async def func(): 7 | async def func():
7 | async with trio.fail_after(): 8 | with trio.fail_after():
| _____^ | _____^
8 | | ... 9 | | ...
| |___________^ ASYNC100 | |___________^ ASYNC100
| |
ASYNC100.py:17:5: ASYNC100 A `with trio.move_on_after(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint. ASYNC100.py:18:5: ASYNC100 A `with trio.move_on_after(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint.
| |
16 | async def func(): 17 | async def func():
17 | async with trio.move_on_after(): 18 | with trio.move_on_after():
| _____^ | _____^
18 | | ... 19 | | ...
| |___________^ ASYNC100 | |___________^ ASYNC100
| |
ASYNC100.py:33:5: ASYNC100 A `with anyio.move_on_after(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint. ASYNC100.py:40:5: ASYNC100 A `with anyio.move_on_after(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint.
| |
32 | async def func(): 39 | async def func():
33 | async with anyio.move_on_after(delay=0.2): 40 | with anyio.move_on_after(delay=0.2):
| _____^ | _____^
34 | | ... 41 | | ...
| |___________^ ASYNC100 | |___________^ ASYNC100
| |
ASYNC100.py:38:5: ASYNC100 A `with anyio.fail_after(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint. ASYNC100.py:45:5: ASYNC100 A `with anyio.fail_after(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint.
| |
37 | async def func(): 44 | async def func():
38 | async with anyio.fail_after(): 45 | with anyio.fail_after():
| _____^ | _____^
39 | | ... 46 | | ...
| |___________^ ASYNC100 | |___________^ ASYNC100
| |
ASYNC100.py:43:5: ASYNC100 A `with anyio.CancelScope(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint. ASYNC100.py:50:5: ASYNC100 A `with anyio.CancelScope(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint.
| |
42 | async def func(): 49 | async def func():
43 | async with anyio.CancelScope(): 50 | with anyio.CancelScope():
| _____^ | _____^
44 | | ... 51 | | ...
| |___________^ ASYNC100 | |___________^ ASYNC100
| |
ASYNC100.py:48:5: ASYNC100 A `with anyio.CancelScope(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint. ASYNC100.py:55:5: ASYNC100 A `with anyio.CancelScope(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint.
| |
47 | async def func(): 54 | async def func():
48 | async with anyio.CancelScope(): 55 | with anyio.CancelScope(), nullcontext():
| _____^ | _____^
49 | | ... 56 | | ...
| |___________^ ASYNC100 | |___________^ ASYNC100
| |
ASYNC100.py:53:5: ASYNC100 A `with asyncio.timeout(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint. ASYNC100.py:60:5: ASYNC100 A `with anyio.CancelScope(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint.
| |
52 | async def func(): 59 | async def func():
53 | async with asyncio.timeout(delay=0.2): 60 | with nullcontext(), anyio.CancelScope():
| _____^ | _____^
54 | | ... 61 | | ...
| |___________^ ASYNC100 | |___________^ ASYNC100
| |
ASYNC100.py:58:5: ASYNC100 A `with asyncio.timeout_at(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint. ASYNC100.py:65:5: ASYNC100 A `with asyncio.timeout(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint.
| |
57 | async def func(): 64 | async def func():
58 | async with asyncio.timeout_at(when=0.2): 65 | async with asyncio.timeout(delay=0.2):
| _____^ | _____^
59 | | ... 66 | | ...
| |___________^ ASYNC100
|
ASYNC100.py:70:5: ASYNC100 A `with asyncio.timeout_at(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint.
|
69 | async def func():
70 | async with asyncio.timeout_at(when=0.2):
| _____^
71 | | ...
| |___________^ ASYNC100
|
ASYNC100.py:80:5: ASYNC100 A `with asyncio.timeout(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint.
|
79 | async def func():
80 | async with asyncio.timeout(delay=0.2), asyncio.TaskGroup(), asyncio.timeout(delay=0.2):
| _____^
81 | | ...
| |___________^ ASYNC100
|
ASYNC100.py:90:5: ASYNC100 A `with asyncio.timeout(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint.
|
89 | async def func():
90 | async with asyncio.timeout(delay=0.2), asyncio.timeout(delay=0.2):
| _____^
91 | | ...
| |___________^ ASYNC100 | |___________^ ASYNC100
| |