Skip to content

Commit

Permalink
[flake8-async] Fix false positives with multiple async with items…
Browse files Browse the repository at this point in the history
… (`ASYNC100`) (#12643)

## Summary

Please see
#12605 (comment) 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.
  • Loading branch information
bluetech authored Aug 2, 2024
1 parent 1c311e4 commit fbfe2cb
Show file tree
Hide file tree
Showing 4 changed files with 126 additions and 78 deletions.
Original file line number Diff line number Diff line change
@@ -1,51 +1,63 @@
import anyio
import asyncio
import trio
from contextlib import nullcontext


async def func():
async with trio.fail_after():
with trio.fail_after():
...


async def func():
async with trio.fail_at():
with trio.fail_at():
await ...


async def func():
async with trio.move_on_after():
with trio.move_on_after():
...


async def func():
async with trio.move_at():
with trio.move_at():
await ...


async def func():
async with trio.move_at():
with trio.move_at():
async with trio.open_nursery():
...


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():
with anyio.fail_after():
...


async def func():
async with anyio.fail_after():
with anyio.CancelScope():
...


async def func():
async with anyio.CancelScope():
with anyio.CancelScope(), nullcontext():
...


async def func():
async with anyio.CancelScope():
with nullcontext(), anyio.CancelScope():
...


Expand All @@ -62,3 +74,18 @@ async def func():
async def func():
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):
...
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ use crate::settings::types::PreviewMode;
///
/// ## Why is this bad?
/// Some asynchronous context managers, such as `asyncio.timeout` and
/// `trio.move_on_after`, have no effect unless they contain an `await`
/// statement. The use of such context managers without an `await` statement is
/// likely a mistake.
/// `trio.move_on_after`, have no effect unless they contain a checkpoint.
/// The use of such context managers without an `await`, `async with` or
/// `async for` statement is likely a mistake.
///
/// ## Example
/// ```python
Expand Down Expand Up @@ -55,17 +55,29 @@ pub(crate) fn cancel_scope_no_checkpoint(
with_stmt: &StmtWith,
with_items: &[WithItem],
) {
let Some(method_name) = with_items.iter().find_map(|item| {
let call = item.context_expr.as_call_expr()?;
let qualified_name = checker
.semantic()
.resolve_qualified_name(call.func.as_ref())?;
MethodName::try_from(&qualified_name)
}) else {
let Some((with_item_pos, method_name)) = with_items
.iter()
.enumerate()
.filter_map(|(pos, item)| {
let call = item.context_expr.as_call_expr()?;
let qualified_name = checker
.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;
};

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;
}

Expand All @@ -76,24 +88,6 @@ pub(crate) fn cancel_scope_no_checkpoint(
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!(method_name.module(), AsyncModule::Trio) {
checker.diagnostics.push(Diagnostic::new(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,20 +1,20 @@
---
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 with trio.fail_after():
7 | async def func():
8 | with trio.fail_after():
| _____^
8 | | ...
9 | | ...
| |___________^ 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 with trio.move_on_after():
17 | async def func():
18 | with trio.move_on_after():
| _____^
18 | | ...
19 | | ...
| |___________^ ASYNC100
|
Original file line number Diff line number Diff line change
@@ -1,74 +1,101 @@
---
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 with trio.fail_after():
7 | async def func():
8 | with trio.fail_after():
| _____^
8 | | ...
9 | | ...
| |___________^ 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 with trio.move_on_after():
17 | async def func():
18 | with trio.move_on_after():
| _____^
18 | | ...
19 | | ...
| |___________^ 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():
33 | async with anyio.move_on_after(delay=0.2):
39 | async def func():
40 | with anyio.move_on_after(delay=0.2):
| _____^
34 | | ...
41 | | ...
| |___________^ 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():
38 | async with anyio.fail_after():
44 | async def func():
45 | with anyio.fail_after():
| _____^
39 | | ...
46 | | ...
| |___________^ 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():
43 | async with anyio.CancelScope():
49 | async def func():
50 | with anyio.CancelScope():
| _____^
44 | | ...
51 | | ...
| |___________^ 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():
48 | async with anyio.CancelScope():
54 | async def func():
55 | with anyio.CancelScope(), nullcontext():
| _____^
49 | | ...
56 | | ...
| |___________^ 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():
53 | async with asyncio.timeout(delay=0.2):
59 | async def func():
60 | with nullcontext(), anyio.CancelScope():
| _____^
54 | | ...
61 | | ...
| |___________^ 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():
58 | async with asyncio.timeout_at(when=0.2):
64 | async def func():
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
|

0 comments on commit fbfe2cb

Please sign in to comment.