Skip to content

Commit

Permalink
[flake8-async] Update ASYNC100 to match upstream (#12221)
Browse files Browse the repository at this point in the history
## Summary

Update the name of `ASYNC100` to match
[upstream](https://flake8-async.readthedocs.io/en/latest/rules.html).

Also update to the functionality to match upstream by supporting
additional context managers from asyncio and anyio. Matching this
[list](https://flake8-async.readthedocs.io/en/latest/glossary.html#timeout-context).

Part of #12039.

## Test Plan

Added the new context managers to the fixture.
  • Loading branch information
augustelalande authored Jul 9, 2024
1 parent 6fa4e32 commit 88abc6a
Show file tree
Hide file tree
Showing 9 changed files with 333 additions and 153 deletions.
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import anyio
import asyncio
import trio


Expand Down Expand Up @@ -25,3 +27,33 @@ async def func():
with trio.move_at():
async with trio.open_nursery() as nursery:
...


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


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


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


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


async def func():
with asyncio.timeout():
...


async def func():
with asyncio.timeout_at():
...
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1313,8 +1313,8 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::UselessWithLock) {
pylint::rules::useless_with_lock(checker, with_stmt);
}
if checker.enabled(Rule::TrioTimeoutWithoutAwait) {
flake8_async::rules::timeout_without_await(checker, with_stmt, items);
if checker.enabled(Rule::CancelScopeNoCheckpoint) {
flake8_async::rules::cancel_scope_no_checkpoint(checker, with_stmt, items);
}
}
Stmt::While(while_stmt @ ast::StmtWhile { body, orelse, .. }) => {
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Pylint, "W3301") => (RuleGroup::Stable, rules::pylint::rules::NestedMinMax),

// flake8-async
(Flake8Async, "100") => (RuleGroup::Stable, rules::flake8_async::rules::TrioTimeoutWithoutAwait),
(Flake8Async, "100") => (RuleGroup::Stable, rules::flake8_async::rules::CancelScopeNoCheckpoint),
(Flake8Async, "105") => (RuleGroup::Stable, rules::flake8_async::rules::TrioSyncCall),
(Flake8Async, "109") => (RuleGroup::Stable, rules::flake8_async::rules::AsyncFunctionWithTimeout),
(Flake8Async, "110") => (RuleGroup::Stable, rules::flake8_async::rules::TrioUnneededSleep),
Expand Down
297 changes: 184 additions & 113 deletions crates/ruff_linter/src/rules/flake8_async/helpers.rs

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion crates/ruff_linter/src/rules/flake8_async/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ mod tests {
use crate::test::test_path;
use crate::{assert_messages, settings};

#[test_case(Rule::TrioTimeoutWithoutAwait, Path::new("ASYNC100.py"))]
#[test_case(Rule::CancelScopeNoCheckpoint, Path::new("ASYNC100.py"))]
#[test_case(Rule::TrioSyncCall, Path::new("ASYNC105.py"))]
#[test_case(Rule::AsyncFunctionWithTimeout, Path::new("ASYNC109_0.py"))]
#[test_case(Rule::AsyncFunctionWithTimeout, Path::new("ASYNC109_1.py"))]
Expand All @@ -38,6 +38,7 @@ mod tests {
Ok(())
}

#[test_case(Rule::CancelScopeNoCheckpoint, Path::new("ASYNC100.py"))]
#[test_case(Rule::AsyncFunctionWithTimeout, Path::new("ASYNC109_0.py"))]
#[test_case(Rule::AsyncFunctionWithTimeout, Path::new("ASYNC109_1.py"))]
fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,40 +3,44 @@ use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::AwaitVisitor;
use ruff_python_ast::visitor::Visitor;
use ruff_python_ast::{StmtWith, WithItem};
use ruff_python_semantic::Modules;

use crate::checkers::ast::Checker;
use crate::rules::flake8_async::helpers::MethodName;
use crate::rules::flake8_async::helpers::{AsyncModule, MethodName};
use crate::settings::types::PreviewMode;

/// ## What it does
/// Checks for trio functions that should contain await but don't.
/// Checks for timeout context managers which do not contain a checkpoint.
///
/// ## Why is this bad?
/// Some trio context managers, such as `trio.fail_after` and
/// 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 functions without an `await` statement is
/// statement. The use of such context managers without an `await` statement is
/// likely a mistake.
///
/// ## Example
/// ```python
/// async def func():
/// with trio.move_on_after(2):
/// with asyncio.timeout(2):
/// do_something()
/// ```
///
/// Use instead:
/// ```python
/// async def func():
/// with trio.move_on_after(2):
/// with asyncio.timeout(2):
/// do_something()
/// await awaitable()
/// ```
///
/// [`asyncio` timeouts]: https://docs.python.org/3/library/asyncio-task.html#timeouts
/// [`anyio` timeouts]: https://anyio.readthedocs.io/en/stable/cancellation.html
/// [`trio` timeouts]: https://trio.readthedocs.io/en/stable/reference-core.html#cancellation-and-timeouts
#[violation]
pub struct TrioTimeoutWithoutAwait {
pub struct CancelScopeNoCheckpoint {
method_name: MethodName,
}

impl Violation for TrioTimeoutWithoutAwait {
impl Violation for CancelScopeNoCheckpoint {
#[derive_message_formats]
fn message(&self) -> String {
let Self { method_name } = self;
Expand All @@ -45,15 +49,11 @@ impl Violation for TrioTimeoutWithoutAwait {
}

/// ASYNC100
pub(crate) fn timeout_without_await(
pub(crate) fn cancel_scope_no_checkpoint(
checker: &mut Checker,
with_stmt: &StmtWith,
with_items: &[WithItem],
) {
if !checker.semantic().seen_module(Modules::TRIO) {
return;
}

let Some(method_name) = with_items.iter().find_map(|item| {
let call = item.context_expr.as_call_expr()?;
let qualified_name = checker
Expand All @@ -64,14 +64,7 @@ pub(crate) fn timeout_without_await(
return;
};

if !matches!(
method_name,
MethodName::MoveOnAfter
| MethodName::MoveOnAt
| MethodName::FailAfter
| MethodName::FailAt
| MethodName::CancelScope
) {
if !method_name.is_timeout_context() {
return;
}

Expand All @@ -81,8 +74,17 @@ pub(crate) fn timeout_without_await(
return;
}

checker.diagnostics.push(Diagnostic::new(
TrioTimeoutWithoutAwait { method_name },
with_stmt.range,
));
if matches!(checker.settings.preview, PreviewMode::Disabled) {
if matches!(method_name.module(), AsyncModule::Trio) {
checker.diagnostics.push(Diagnostic::new(
CancelScopeNoCheckpoint { method_name },
with_stmt.range,
));
}
} else {
checker.diagnostics.push(Diagnostic::new(
CancelScopeNoCheckpoint { method_name },
with_stmt.range,
));
}
}
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/rules/flake8_async/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ pub(crate) use blocking_http_call::*;
pub(crate) use blocking_open_call::*;
pub(crate) use blocking_process_invocation::*;
pub(crate) use blocking_sleep::*;
pub(crate) use cancel_scope_no_checkpoint::*;
pub(crate) use sleep_forever_call::*;
pub(crate) use sync_call::*;
pub(crate) use timeout_without_await::*;
pub(crate) use unneeded_sleep::*;
pub(crate) use zero_sleep_call::*;

Expand All @@ -14,8 +14,8 @@ mod blocking_http_call;
mod blocking_open_call;
mod blocking_process_invocation;
mod blocking_sleep;
mod cancel_scope_no_checkpoint;
mod sleep_forever_call;
mod sync_call;
mod timeout_without_await;
mod unneeded_sleep;
mod zero_sleep_call;
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:5: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: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.
|
4 | async def func():
5 | with trio.fail_after():
6 | async def func():
7 | with trio.fail_after():
| _____^
6 | | ...
8 | | ...
| |___________^ ASYNC100
|

ASYNC100.py:15: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: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.
|
14 | async def func():
15 | with trio.move_on_after():
16 | async def func():
17 | with trio.move_on_after():
| _____^
16 | | ...
18 | | ...
| |___________^ ASYNC100
|
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
---
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():
| _____^
8 | | ...
| |___________^ 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.
|
16 | async def func():
17 | with trio.move_on_after():
| _____^
18 | | ...
| |___________^ 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.
|
32 | async def func():
33 | with anyio.move_on_after():
| _____^
34 | | ...
| |___________^ 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.
|
37 | async def func():
38 | with anyio.fail_after():
| _____^
39 | | ...
| |___________^ 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.
|
42 | async def func():
43 | with anyio.CancelScope():
| _____^
44 | | ...
| |___________^ 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.
|
47 | async def func():
48 | with anyio.CancelScope():
| _____^
49 | | ...
| |___________^ 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.
|
52 | async def func():
53 | with asyncio.timeout():
| _____^
54 | | ...
| |___________^ 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.
|
57 | async def func():
58 | with asyncio.timeout_at():
| _____^
59 | | ...
| |___________^ ASYNC100
|

0 comments on commit 88abc6a

Please sign in to comment.