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

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

Merged
merged 2 commits into from
Aug 2, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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():
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The trio and anyio context managers are not async. The difference is relevant for this change, so I changed them back to with, this way we test both with and async with (asyncio).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, that's my bad!

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,13 @@ 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():
...
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,92 @@
---
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
|
Loading