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.

Sign up for GitHub

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

Jump to bottom

Avoid unused async when context manager includes TaskGroup #12605

Merged
merged 1 commit into from
Aug 1, 2024
+46 −22
Merged

Avoid unused async when context manager includes TaskGroup #12605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
29 changes: 17 additions & 12 deletions crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC100.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,56 +4,61 @@


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


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


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


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


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


async def func():
with anyio.move_on_after():
async with anyio.move_on_after(delay=0.2):
...


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


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


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


async def func():
with asyncio.timeout():
async with asyncio.timeout(delay=0.2):
...


async def func():
with asyncio.timeout_at():
async with asyncio.timeout_at(when=0.2):
...


async def func():
async with asyncio.timeout(delay=0.2), asyncio.TaskGroup():
...
19 changes: 19 additions & 0 deletions crates/ruff_linter/src/rules/flake8_async/rules/cancel_scope_no_checkpoint.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,31 @@ pub(crate) fn cancel_scope_no_checkpoint(
return;
}

// If the body contains an `await` statement, the context manager is used correctly.
let mut visitor = AwaitVisitor::default();
visitor.visit_body(&with_stmt.body);
if visitor.seen_await {
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"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it hold for any async context manager? I don't think there is something unique about asyncio.TaskGroup.

If the async with is explicitly nested then ruff doesn't trigger the error. So perhaps the fix should be to make

async with asyncio.timeout(), foo():
    ...

be treated the same as

async with asyncio.timeout():
    async with foo():
        ...

According to the Language Reference they are semantically equivalent (this is for with, the async with ref seems to have an omission here):

image

I also checked async for and it's detected correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you're right.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interested in changing it? :) We might need to accommodate cases like async with asyncio.timeout(), asyncio.timeout(), I guess those should still trigger even if weird.

})
})
}) {
return;
}

if matches!(checker.settings.preview, PreviewMode::Disabled) {
if matches!(method_name.module(), AsyncModule::Trio) {
checker.diagnostics.push(Diagnostic::new(
Expand Down
4 changes: 2 additions & 2 deletions ...flake8_async/snapshots/ruff_linter__rules__flake8_async__tests__ASYNC100_ASYNC100.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ 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.
|
6 | async def func():
7 | with trio.fail_after():
7 | async with trio.fail_after():
| _____^
8 | | ...
| |___________^ ASYNC100
Expand All @@ -13,7 +13,7 @@ ASYNC100.py:7:5: ASYNC100 A `with trio.fail_after(...):` context does not contai
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.
|
16 | async def func():
17 | with trio.move_on_after():
17 | async with trio.move_on_after():
| _____^
18 | | ...
| |___________^ ASYNC100
Expand Down
16 changes: 8 additions & 8 deletions ...ync/snapshots/ruff_linter__rules__flake8_async__tests__preview__ASYNC100_ASYNC100.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ 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.
|
6 | async def func():
7 | with trio.fail_after():
7 | async with trio.fail_after():
| _____^
8 | | ...
| |___________^ ASYNC100
Expand All @@ -13,7 +13,7 @@ ASYNC100.py:7:5: ASYNC100 A `with trio.fail_after(...):` context does not contai
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.
|
16 | async def func():
17 | with trio.move_on_after():
17 | async with trio.move_on_after():
| _____^
18 | | ...
| |___________^ ASYNC100
Expand All @@ -22,7 +22,7 @@ ASYNC100.py:17:5: ASYNC100 A `with trio.move_on_after(...):` context does not co
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.
|
32 | async def func():
33 | with anyio.move_on_after():
33 | async with anyio.move_on_after(delay=0.2):
| _____^
34 | | ...
| |___________^ ASYNC100
Expand All @@ -31,7 +31,7 @@ ASYNC100.py:33:5: ASYNC100 A `with anyio.move_on_after(...):` context does not c
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.
|
37 | async def func():
38 | with anyio.fail_after():
38 | async with anyio.fail_after():
| _____^
39 | | ...
| |___________^ ASYNC100
Expand All @@ -40,7 +40,7 @@ ASYNC100.py:38:5: ASYNC100 A `with anyio.fail_after(...):` context does not cont
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.
|
42 | async def func():
43 | with anyio.CancelScope():
43 | async with anyio.CancelScope():
| _____^
44 | | ...
| |___________^ ASYNC100
Expand All @@ -49,7 +49,7 @@ ASYNC100.py:43:5: ASYNC100 A `with anyio.CancelScope(...):` context does not con
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.
|
47 | async def func():
48 | with anyio.CancelScope():
48 | async with anyio.CancelScope():
| _____^
49 | | ...
| |___________^ ASYNC100
Expand All @@ -58,7 +58,7 @@ ASYNC100.py:48:5: ASYNC100 A `with anyio.CancelScope(...):` context does not con
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.
|
52 | async def func():
53 | with asyncio.timeout():
53 | async with asyncio.timeout(delay=0.2):
| _____^
54 | | ...
| |___________^ ASYNC100
Expand All @@ -67,7 +67,7 @@ ASYNC100.py:53:5: ASYNC100 A `with asyncio.timeout(...):` context does not conta
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.
|
57 | async def func():
58 | with asyncio.timeout_at():
58 | async with asyncio.timeout_at(when=0.2):
| _____^
59 | | ...
| |___________^ ASYNC100
Expand Down
Loading